diff --git a/libraries/validation-tools/test/unit/src/zodHelpers.test.ts b/libraries/validation-tools/test/unit/src/zodHelpers.test.ts index 427eb66572..98d75ab3ef 100644 --- a/libraries/validation-tools/test/unit/src/zodHelpers.test.ts +++ b/libraries/validation-tools/test/unit/src/zodHelpers.test.ts @@ -217,4 +217,121 @@ describe('zodHelpers', () => { ]) }) }) + describe('buildId', () => { + it('fails to parse when provided with an invalid buildId', () => { + const parsed = zz.buildId().safeParse('aa') + expect(parsed.success).toBe(false) + expect(parsed.error?.issues).toHaveLength(1) + expect(parsed.error?.issues).toMatchObject([ + expect.objectContaining({ + message: 'invalid buildId', + }), + ]) + }) + + it('parses successfully when provided with a valid buildId', () => { + const parsed = zz.buildId().safeParse('19d6c341530-878fff6cdab7fb0c') + expect(parsed.success).toBe(true) + expect(parsed.data).toBe('19d6c341530-878fff6cdab7fb0c') + }) + + it('fails to parse when provided with an editorBuildId', () => { + const parsed = zz + .buildId() + .safeParse( + '03b1d773-6203-4669-b365-6a0aa5625878-19d6c341530-878fff6cdab7fb0c' + ) + expect(parsed.success).toBe(false) + expect(parsed.error?.issues).toHaveLength(1) + expect(parsed.error?.issues).toMatchObject([ + expect.objectContaining({ + message: 'invalid buildId', + }), + ]) + }) + }) + + describe('editorBuildId', () => { + it('fails to parse when provided with an invalid buildId', () => { + const parsed = zz.editorBuildId().safeParse('aa') + expect(parsed.success).toBe(false) + expect(parsed.error?.issues).toHaveLength(1) + expect(parsed.error?.issues).toMatchObject([ + expect.objectContaining({ + message: 'invalid editorId-buildId', + }), + ]) + }) + + it('fails to parse when provided with a buildId', () => { + const parsed = zz + .editorBuildId() + .safeParse('19d6c341530-878fff6cdab7fb0c') + expect(parsed.success).toBe(false) + expect(parsed.error?.issues).toHaveLength(1) + expect(parsed.error?.issues).toMatchObject([ + expect.objectContaining({ + message: 'invalid editorId-buildId', + }), + ]) + }) + + it('parses successfully when provided with a valid editorId-buildId', () => { + const parsed = zz + .editorBuildId() + .safeParse( + '03b1d773-6203-4669-b365-6a0aa5625878-19d6c341530-878fff6cdab7fb0c' + ) + expect(parsed.success).toBe(true) + expect(parsed.data).toBe( + '03b1d773-6203-4669-b365-6a0aa5625878-19d6c341530-878fff6cdab7fb0c' + ) + }) + }) + describe('filepath', () => { + it('fails to parse with empty input', () => { + const parsed = zz.filepath().safeParse('') + expect(parsed.success).toBe(false) + expect(parsed.error?.issues).toHaveLength(1) + expect(parsed.error?.issues).toMatchObject([ + expect.objectContaining({ + message: 'path is empty', + }), + ]) + }) + + it('fails to parse with absolute path', () => { + const parsed = zz.filepath().safeParse('/output.pdf') + expect(parsed.success).toBe(false) + expect(parsed.error?.issues).toHaveLength(1) + expect(parsed.error?.issues).toMatchObject([ + expect.objectContaining({ + message: 'path is absolute', + }), + ]) + }) + + it('fails to parse when provided with path traversal', () => { + const parsed = zz.filepath().safeParse('../output.pdf') + expect(parsed.success).toBe(false) + expect(parsed.error?.issues).toHaveLength(1) + expect(parsed.error?.issues).toMatchObject([ + expect.objectContaining({ + message: 'path traversal detected', + }), + ]) + }) + + it('parses successfully when provided a valid path', () => { + const parsed = zz.filepath().safeParse('output.pdf') + expect(parsed.success).toBe(true) + expect(parsed.data).toBe('output.pdf') + }) + + it('parses successfully when provided a valid nested path', () => { + const parsed = zz.filepath().safeParse('foo/output.pdf') + expect(parsed.success).toBe(true) + expect(parsed.data).toBe('foo/output.pdf') + }) + }) }) diff --git a/libraries/validation-tools/testUtils.js b/libraries/validation-tools/testUtils.js new file mode 100644 index 0000000000..66246031fd --- /dev/null +++ b/libraries/validation-tools/testUtils.js @@ -0,0 +1,8 @@ +function asZodError(...def) { + return { + name: 'ZodError', + _zod: { def }, + } +} + +module.exports = { asZodError } diff --git a/libraries/validation-tools/zodHelpers.js b/libraries/validation-tools/zodHelpers.js index 6a45d7beaa..b20bf7fc6c 100644 --- a/libraries/validation-tools/zodHelpers.js +++ b/libraries/validation-tools/zodHelpers.js @@ -33,6 +33,31 @@ const zz = { datetimeNullable: options => datetimeSchema({ ...options, allowNull: true }), datetimeNullish: options => datetimeSchema({ ...options, allowNull: true, allowUndefined: true }), + buildId: () => + z.string().regex(/^[0-9a-f]+-[0-9a-f]+$/, { message: 'invalid buildId' }), + editorBuildId: () => + z.string().regex(/^[a-f0-9-]{36}-[0-9a-f]+-[0-9a-f]+$/, { + message: 'invalid editorId-buildId', + }), + clsiServerId: () => + z.string().regex(/^[a-z0-9-]+$/, { message: 'invalid clsiServerId' }), + compileBackendClass: () => + z + .string() + .regex(/^[a-z0-9-]+$/, { message: 'invalid compileBackendClass' }), + compileGroup: () => + z.enum(['alpha', 'gvisor', 'standard', 'priority'], { + message: 'invalid compileGroup', + }), + submissionId: () => z.string().regex(/^[a-zA-Z0-9_-]+$/), + filepath: () => + z + .string() + .nonempty({ message: 'path is empty' }) + .refine(s => !s.startsWith('/'), { message: 'path is absolute' }) + .refine(s => !s.split('/').includes('..'), { + message: 'path traversal detected', + }), } module.exports = { zz } diff --git a/services/clsi/test/unit/js/CompileController.test.js b/services/clsi/test/unit/js/CompileController.test.js index c293b5d220..895f040eec 100644 --- a/services/clsi/test/unit/js/CompileController.test.js +++ b/services/clsi/test/unit/js/CompileController.test.js @@ -82,12 +82,12 @@ describe('CompileController', () => { path: 'output.pdf', type: 'pdf', size: 1337, - build: 1234, + build: '1234-5678', }, { path: 'output.log', type: 'log', - build: 1234, + build: '1234-5678', }, ] ctx.RequestParser.parse = sinon.stub().callsArgWith(1, null, ctx.request) @@ -190,12 +190,12 @@ describe('CompileController', () => { { path: 'fake_output.pdf', type: 'pdf', - build: 1234, + build: '1234-5678', }, { path: 'output.log', type: 'log', - build: 1234, + build: '1234-5678', }, ] ctx.CompileManager.doCompileWithLock = sinon @@ -239,12 +239,12 @@ describe('CompileController', () => { path: 'output.pdf', type: 'pdf', size: 0, - build: 1234, + build: '1234-5678', }, { path: 'output.log', type: 'log', - build: 1234, + build: '1234-5678', }, ] ctx.CompileManager.doCompileWithLock = sinon diff --git a/services/clsi/test/unit/js/CompileManager.test.js b/services/clsi/test/unit/js/CompileManager.test.js index 150da1e61d..1304e1541e 100644 --- a/services/clsi/test/unit/js/CompileManager.test.js +++ b/services/clsi/test/unit/js/CompileManager.test.js @@ -27,12 +27,12 @@ describe('CompileManager', () => { { path: 'output.log', type: 'log', - build: 1234, + build: '1234-5678', }, { path: 'output.pdf', type: 'pdf', - build: 1234, + build: '1234-5678', }, ] ctx.buildId = '00000000000-0000000000000000' diff --git a/services/web/app/src/Features/Compile/ClsiCacheController.mjs b/services/web/app/src/Features/Compile/ClsiCacheController.mjs index b87d290487..92ee7b2b20 100644 --- a/services/web/app/src/Features/Compile/ClsiCacheController.mjs +++ b/services/web/app/src/Features/Compile/ClsiCacheController.mjs @@ -13,6 +13,18 @@ import ClsiCacheHandler from './ClsiCacheHandler.mjs' import ProjectGetter from '../Project/ProjectGetter.mjs' import { MeteredStream } from '@overleaf/stream-utils' import Metrics from '@overleaf/metrics' +import { z, zz } from '@overleaf/validation-tools' +import { parseReq } from '../../infrastructure/Validation.mjs' + +const downloadFromCacheSchema = z.object({ + params: z.object({ + Project_id: zz.objectId(), + editorBuildId: zz.editorBuildId(), + filename: zz.filepath().refine(s => ClsiCacheHandler.isAllowedFilename(s), { + message: 'path is not allowed', + }), + }), +}) /** * Download a file from a specific build on the clsi-cache. @@ -22,12 +34,14 @@ import Metrics from '@overleaf/metrics' * @return {Promise<*>} */ async function downloadFromCache(req, res) { - const { Project_id: projectId, buildId, filename } = req.params + const { + params: { Project_id: projectId, editorBuildId, filename }, + } = parseReq(req, downloadFromCacheSchema) return await _downloadFromCacheWithParams( req, res, projectId, - buildId, + editorBuildId, filename ) } @@ -38,18 +52,16 @@ async function downloadFromCache(req, res) { * @param req * @param res * @param projectId - * @param buildId + * @param editorBuildId * @param filename * @param projectId - * @param buildId - * @param filename * @return {Promise<*>} */ async function _downloadFromCacheWithParams( req, res, projectId, - buildId, + editorBuildId, filename ) { const userId = CompileController._getUserIdForCompile(req) @@ -61,7 +73,7 @@ async function _downloadFromCacheWithParams( ClsiCacheHandler.getOutputFile( projectId, userId, - buildId, + editorBuildId, filename, ac.signal ), diff --git a/services/web/app/src/Features/Compile/ClsiCacheHandler.mjs b/services/web/app/src/Features/Compile/ClsiCacheHandler.mjs index ac1960de93..c37648dc7f 100644 --- a/services/web/app/src/Features/Compile/ClsiCacheHandler.mjs +++ b/services/web/app/src/Features/Compile/ClsiCacheHandler.mjs @@ -10,6 +10,7 @@ import OError from '@overleaf/o-error' import { NotFoundError, InvalidNameError } from '../Errors/Errors.js' import Features from '../../infrastructure/Features.mjs' import Path from 'node:path' +import { zz } from '@overleaf/validation-tools' const TIMEOUT = 4_000 @@ -18,6 +19,29 @@ const TIMEOUT = 4_000 */ const lastFailures = new Map() +/** + * Keep in sync with isAllowedFilename in services/clsi-cache/app/js/utils.js + * + * @param {string} filename + * @return {boolean} + */ +function isAllowedFilename(filename) { + return ( + [ + 'output.blg', + 'output.log', + 'output.pdf', + 'output.synctex.gz', + 'output.overleaf.json', + 'output.tar.gz', + // Not in web: 'history-resync.json.gz' is only read/written by clsi. + // The user/frontend should not be able to download it directly. + // We need to block access to it on the web layer. + // If we ever remove blockRestrictedUserFromProject from the history endpoints, we can remove this restriction. + ].includes(filename) || filename.endsWith('.blg') + ) +} + /** * Keep in sync with validateFilename in services/clsi-cache/app/js/utils.js * @@ -27,18 +51,7 @@ function validateFilename(filename) { if (filename.split('/').includes('..')) { throw new InvalidNameError('path traversal') } - if ( - !( - [ - 'output.blg', - 'output.log', - 'output.pdf', - 'output.synctex.gz', - 'output.overleaf.json', - 'output.tar.gz', - ].includes(filename) || filename.endsWith('.blg') - ) - ) { + if (!isAllowedFilename(filename)) { throw new InvalidNameError('bad filename') } } @@ -90,12 +103,14 @@ async function clearCache(projectId, userId) { ) } +const editorBuildIdSchema = zz.editorBuildId() + /** * Get an output file from a specific build. * * @param projectId * @param userId - * @param buildId + * @param editorBuildId * @param filename * @param signal * @return {Promise<{size: number, zone: string, shard: string, location: string, lastModified: Date, allFiles: string[]}>} @@ -103,20 +118,18 @@ async function clearCache(projectId, userId) { async function getOutputFile( projectId, userId, - buildId, + editorBuildId, filename, signal = AbortSignal.timeout(TIMEOUT) ) { validateFilename(filename) - if (!/^[a-f0-9-]+$/.test(buildId)) { - throw new InvalidNameError('bad buildId') - } + editorBuildId = editorBuildIdSchema.parse(editorBuildId) let path = `/project/${projectId}` if (userId) { path += `/user/${userId}` } - path += `/build/${buildId}/search/output/${filename}` + path += `/build/${editorBuildId}/search/output/${filename}` return getRedirectWithFallback(projectId, userId, path, signal) } @@ -269,7 +282,7 @@ async function prepareCacheSource( * * @param clsiCacheShard * @param submissionId - * @param buildId + * @param editorBuildId * @param templateVersionId * @param imageName * @return {Promise} @@ -277,13 +290,13 @@ async function prepareCacheSource( async function exportSubmissionAsTemplate( clsiCacheShard, submissionId, - buildId, + editorBuildId, templateVersionId, imageName ) { imageName = Path.basename(imageName) const url = new URL( - `/submission/${submissionId}/build/${buildId}/export-as-template`, + `/submission/${submissionId}/build/${editorBuildId}/export-as-template`, Settings.apis.clsiCache.instances.find(i => i.shard === clsiCacheShard).url ) try { @@ -306,6 +319,7 @@ async function exportSubmissionAsTemplate( export default { TIMEOUT, + isAllowedFilename, getEgressLabel, clearCache, getOutputFile, diff --git a/services/web/app/src/Features/Compile/ClsiManager.mjs b/services/web/app/src/Features/Compile/ClsiManager.mjs index 97f4a93c5a..1b9a28dc34 100644 --- a/services/web/app/src/Features/Compile/ClsiManager.mjs +++ b/services/web/app/src/Features/Compile/ClsiManager.mjs @@ -24,6 +24,7 @@ import HistoryManager from '../History/HistoryManager.mjs' import SplitTestHandler from '../SplitTests/SplitTestHandler.mjs' import AnalyticsManager from '../Analytics/AnalyticsManager.mjs' import RedisWrapper from '../../infrastructure/RedisWrapper.mjs' +import { getOutputFileURL } from './ClsiURLHelpers.mjs' // use the redis db with eviction policy enabled const rclient = RedisWrapper.client('clsi_cookie') @@ -850,17 +851,17 @@ async function getContentFromDocUpdaterIfMatch(projectId, project, options) { async function getOutputFileStream( projectId, userId, - options, clsiServerId, buildId, outputFilePath ) { - const { compileBackendClass, compileGroup } = options - const url = new URL(Settings.apis.clsi.downloadHost) - url.pathname = `/project/${projectId}/user/${userId}/build/${buildId}/output/${outputFilePath}` - url.searchParams.set('compileBackendClass', compileBackendClass) - url.searchParams.set('compileGroup', compileGroup) - url.searchParams.set('clsiserverid', clsiServerId) + const url = getOutputFileURL( + projectId, + userId, + buildId, + outputFilePath, + clsiServerId + ) try { const stream = await fetchStream(url, { signal: AbortSignal.timeout(OUTPUT_FILE_TIMEOUT_MS), diff --git a/services/web/app/src/Features/Compile/ClsiURLHelpers.mjs b/services/web/app/src/Features/Compile/ClsiURLHelpers.mjs new file mode 100644 index 0000000000..7c28dc1997 --- /dev/null +++ b/services/web/app/src/Features/Compile/ClsiURLHelpers.mjs @@ -0,0 +1,85 @@ +import { zz } from '@overleaf/validation-tools' +import Settings from '@overleaf/settings' + +// Build zod schema once and use it below. +const schema = { + compileBackendClass: zz.compileBackendClass(), + optionalClsiServerId: zz.clsiServerId().optional(), + projectIdOrSubmissionId: zz.objectId().or(zz.submissionId()), + optionalUserId: zz.objectId().optional(), + buildId: zz.buildId(), + filepath: zz.filepath(), +} + +/** + * @param {string} projectIdOrSubmissionId + * @param {string|null} userId + * @param {string} buildId + * @param {string} compileBackendClass + * @param {string} clsiServerId + * @return {URL} + */ +export function getOutputZipURL( + projectIdOrSubmissionId, + userId, + buildId, + compileBackendClass, + clsiServerId +) { + compileBackendClass = schema.compileBackendClass.parse(compileBackendClass) + clsiServerId = schema.optionalClsiServerId.parse(clsiServerId) + const url = new URL(Settings.apis.clsi.url) + url.pathname = getFilePath( + projectIdOrSubmissionId, + userId, + buildId, + 'output.zip' + ) + url.searchParams.set('compileBackendClass', compileBackendClass) + if (clsiServerId) url.searchParams.set('clsiserverid', clsiServerId) + return url +} + +/** + * @param {string} projectIdOrSubmissionId + * @param {string|null} userId + * @param {string} buildId + * @param {string} file + * @param {string} clsiServerId + * @return {URL} + */ +export function getOutputFileURL( + projectIdOrSubmissionId, + userId, + buildId, + file, + clsiServerId +) { + clsiServerId = schema.optionalClsiServerId.parse(clsiServerId) + const url = new URL(Settings.apis.clsi.downloadHost) + url.pathname = getFilePath(projectIdOrSubmissionId, userId, buildId, file) + if (clsiServerId) url.searchParams.set('clsiserverid', clsiServerId) + return url +} + +/** + * @param {string} projectIdOrSubmissionId + * @param {string|null} userId + * @param {string} buildId + * @param {string} file + * @return {string} + */ +export function getFilePath(projectIdOrSubmissionId, userId, buildId, file) { + projectIdOrSubmissionId = schema.projectIdOrSubmissionId.parse( + projectIdOrSubmissionId.toString() + ) + userId = schema.optionalUserId.parse(userId?.toString()) + buildId = schema.buildId.parse(buildId) + file = schema.filepath.parse(file) + let path = `/project/${projectIdOrSubmissionId}` + if (userId) { + path += `/user/${userId}` + } + path += `/build/${buildId}/output/${file}` + return path +} diff --git a/services/web/app/src/Features/Compile/CompileController.mjs b/services/web/app/src/Features/Compile/CompileController.mjs index cae9e0712b..a40c2bc3a7 100644 --- a/services/web/app/src/Features/Compile/CompileController.mjs +++ b/services/web/app/src/Features/Compile/CompileController.mjs @@ -1,7 +1,5 @@ import { URL } from 'node:url' import { pipeline } from 'node:stream/promises' -import { Cookie } from 'tough-cookie' -import OError from '@overleaf/o-error' import Metrics from '@overleaf/metrics' import ProjectGetter from '../Project/ProjectGetter.mjs' import CompileManager from './CompileManager.mjs' @@ -12,7 +10,6 @@ import Errors from '../Errors/Errors.js' import SessionManager from '../Authentication/SessionManager.mjs' import { RateLimiter } from '../../infrastructure/RateLimiter.mjs' import Validation from '../../infrastructure/Validation.mjs' -import ClsiCookieManagerFactory from './ClsiCookieManager.mjs' import Path from 'node:path' import AnalyticsManager from '../Analytics/AnalyticsManager.mjs' import SplitTestHandler from '../SplitTests/SplitTestHandler.mjs' @@ -24,16 +21,17 @@ import { import Features from '../../infrastructure/Features.mjs' import ClsiCacheController from './ClsiCacheController.mjs' import { prepareZipAttachment } from '../../infrastructure/Response.mjs' +import ClsiCacheHandler from './ClsiCacheHandler.mjs' +import { + getFilePath, + getOutputFileURL, + getOutputZipURL, +} from './ClsiURLHelpers.mjs' const { z, zz, parseReq } = Validation -const ClsiCookieManager = ClsiCookieManagerFactory( - Settings.apis.clsi?.backendGroupName -) const COMPILE_TIMEOUT_MS = 12 * 60 * 1000 -const buildIdSchema = z.string().regex(/[a-z0-9-]/) - const pdfDownloadRateLimiter = new RateLimiter('full-pdf-download', { points: 1000, duration: 60 * 60, @@ -43,7 +41,7 @@ function getOutputFilesArchiveSpecification(projectId, userId, buildId) { const fileName = 'output.zip' return { path: fileName, - url: _CompileController._getFileUrl(projectId, userId, buildId, fileName), + url: getFilePath(projectId, userId, buildId, fileName), type: 'zip', } } @@ -119,7 +117,7 @@ const deleteAuxFilesSchema = z.object({ Project_id: zz.objectId(), }), query: z.object({ - clsiserverid: z.string().optional(), + clsiserverid: zz.clsiServerId().optional(), }), }) @@ -128,11 +126,55 @@ const wordCountSchema = z.object({ Project_id: zz.objectId(), }), query: z.object({ - clsiserverid: z.string().optional(), + clsiserverid: zz.clsiServerId().optional(), file: z.string().optional(), }), }) +const getFileForSubmissionFromClsiSchema = z.object({ + params: z.object({ + submissionId: zz.submissionId(), + build_id: zz.buildId(), + file: zz.filepath(), + }), + query: z.object({ + clsiserverid: zz.clsiServerId().optional(), + }), +}) + +const getFileFromClsiSchema = z.object({ + params: z.object({ + Project_id: zz.objectId(), + build_id: zz.buildId(), + file: zz.filepath(), + }), + query: z.object({ + clsiserverid: zz.clsiServerId().optional(), + editorId: z.uuid().optional(), + }), +}) + +const getOutputPDFFromClsiSchema = z.object({ + params: z.object({ + Project_id: zz.objectId(), + build_id: zz.buildId(), + }), + query: z.object({ + clsiserverid: zz.clsiServerId().optional(), + editorId: z.uuid().optional(), + }), +}) + +const getOutputZipFromClsiSchema = z.object({ + params: z.object({ + Project_id: zz.objectId(), + build_id: zz.buildId(), + }), + query: z.object({ + clsiserverid: zz.clsiServerId().optional(), + }), +}) + const _CompileController = { async compile(req, res) { res.setTimeout(COMPILE_TIMEOUT_MS) @@ -331,18 +373,23 @@ const _CompileController = { }, async downloadPdf(req, res) { + const { + params: { Project_id: projectId, build_id: buildId }, + query: { clsiserverid: clsiServerId, editorId }, + } = parseReq(req, getOutputPDFFromClsiSchema) Metrics.inc('pdf-downloads') - const projectId = req.params.Project_id - const rateLimit = () => - pdfDownloadRateLimiter - .consume(req.ip, 1, { method: 'ip' }) - .then(() => true) - .catch(err => { - if (err instanceof Error) { - throw err - } - return false - }) + try { + await pdfDownloadRateLimiter.consume(req.ip, 1, { method: 'ip' }) + } catch (err) { + if (err instanceof Error) { + logger.err({ err }, 'error checking rate limit for pdf download') + res.status(500).end() + return + } + logger.debug({ projectId, ip: req.ip }, 'rate limit hit downloading pdf') + res.status(429).end() + return + } const project = await ProjectGetter.promises.getProject(projectId, { name: 1, @@ -357,38 +404,18 @@ const _CompileController = { res.setContentDisposition('inline', { filename }) } - let canContinue - try { - canContinue = await rateLimit() - } catch (err) { - logger.err({ err }, 'error checking rate limit for pdf download') - res.sendStatus(500) - return - } - - if (!canContinue) { - logger.debug({ projectId, ip: req.ip }, 'rate limit hit downloading pdf') - res.sendStatus(500) // should it be 429? - } else { - const userId = CompileController._getUserIdForCompile(req) - - const url = _CompileController._getFileUrl( - projectId, - userId, - req.params.build_id, - 'output.pdf' - ) - // Align params with the generic output file download (via getFileFromClsi / getFileFromClsiWithoutUser). - req.params.file = 'output.pdf' - await CompileController._proxyToClsi( - projectId, - 'output-file', - url, - {}, - req, - res - ) - } + const userId = CompileController._getUserIdForCompile(req) + await _downloadFromClsiNginx( + projectId, + userId, + editorId, + buildId, + 'output.pdf', + clsiServerId, + 'output-file', + req, + res + ) }, // Keep in sync with the logic for zip files in ProjectDownloadsController @@ -397,10 +424,11 @@ const _CompileController = { }, async deleteAuxFiles(req, res) { - const { params, query } = parseReq(req, deleteAuxFilesSchema) - const projectId = params.Project_id - const { clsiserverid } = query - const userId = await CompileController._getUserIdForCompile(req) + const { + params: { Project_id: projectId }, + query: { clsiserverid }, + } = parseReq(req, deleteAuxFilesSchema) + const userId = CompileController._getUserIdForCompile(req) await CompileManager.promises.deleteAuxFiles( projectId, userId, @@ -413,9 +441,9 @@ const _CompileController = { async compileAndDownloadPdf(req, res) { const projectId = req.params.project_id - let outputFiles + let outputFiles, clsiServerId, buildId try { - ;({ outputFiles } = await CompileManager.promises + ;({ outputFiles, clsiServerId, buildId } = await CompileManager.promises // pass userId as null, since templates are an "anonymous" compile .compile(projectId, null, {})) } catch (err) { @@ -435,19 +463,25 @@ const _CompileController = { res.sendStatus(500) return } - await CompileController._proxyToClsi( + await _downloadFromClsiNginx( projectId, + null, + null, + buildId, + 'output.pdf', + clsiServerId, 'output-file', - pdf.url, - {}, req, res ) }, async getOutputZipFromClsi(req, res) { - const projectId = req.params.Project_id const userId = CompileController._getUserIdForCompile(req) + const { + params: { Project_id: projectId, build_id: buildId }, + query: { clsiserverid: clsiServerId }, + } = parseReq(req, getOutputZipFromClsiSchema) const project = await ProjectGetter.promises.getProject(projectId, { name: 1, @@ -455,87 +489,57 @@ const _CompileController = { const filename = `${_CompileController._getSafeProjectName(project)}-output.zip` prepareZipAttachment(res, filename) - const qs = {} - const url = _CompileController._getFileUrl( + await _downloadFromClsi( projectId, userId, - req.params.build_id, - 'output.zip' - ) - await CompileController._proxyToClsi( - projectId, + null, + buildId, + 'output.zip', + clsiServerId, 'output-zip-file', - url, - qs, req, res ) }, async getFileFromClsi(req, res) { - const projectId = req.params.Project_id const userId = CompileController._getUserIdForCompile(req) + const { + params: { Project_id: projectId, build_id: buildId, file }, + query: { clsiserverid: clsiServerId, editorId }, + } = parseReq(req, getFileFromClsiSchema) - const qs = {} - - const url = _CompileController._getFileUrl( + await _downloadFromClsiNginx( projectId, userId, - req.params.build_id, - req.params.file - ) - await CompileController._proxyToClsi( - projectId, + editorId, + buildId, + file, + clsiServerId, 'output-file', - url, - qs, req, res ) }, - async getFileFromClsiWithoutUser(req, res) { - const submissionId = req.params.submission_id - const url = _CompileController._getFileUrl( + async getFileForSubmissionFromClsi(req, res) { + const { + params: { submissionId, build_id: buildId, file }, + query: { clsiserverid: clsiServerId }, + } = parseReq(req, getFileForSubmissionFromClsiSchema) + await _downloadFromClsiNginx( submissionId, null, - req.params.build_id, - req.params.file - ) - const limits = { - compileGroup: - req.body?.compileGroup || - req.query?.compileGroup || - Settings.defaultFeatures.compileGroup, - compileBackendClass: Settings.apis.clsi.submissionBackendClass, - } - await CompileController._proxyToClsiWithLimits( - submissionId, + null, + buildId, + file, + clsiServerId, 'output-file', - url, - {}, - limits, req, res ) }, - // compute a GET file url for a given project, user (optional), build (optional) and file - _getFileUrl(projectId, userId, buildId, file) { - let url - if (userId != null && buildId != null) { - url = `/project/${projectId}/user/${userId}/build/${buildId}/output/${file}` - } else if (userId != null) { - url = `/project/${projectId}/user/${userId}/output/${file}` - } else if (buildId != null) { - buildId = buildIdSchema.parse(buildId) - url = `/project/${projectId}/build/${buildId}/output/${file}` - } else { - url = `/project/${projectId}/output/${file}` - } - return url - }, - async proxySyncPdf(req, res) { const { page, h, v } = req.query if (!page?.match(/^\d+$/)) { @@ -573,163 +577,6 @@ const _CompileController = { await _syncTeX(req, res, 'code', { file, line, column }) }, - async _proxyToClsi(projectId, action, url, qs, req, res) { - const limits = - await CompileManager.promises.getProjectCompileLimits(projectId) - return CompileController._proxyToClsiWithLimits( - projectId, - action, - url, - qs, - limits, - req, - res - ) - }, - - async _proxyToClsiWithLimits( - projectId, - action, - requestPath, - qs, - limits, - req, - res - ) { - const persistenceOptions = await _getPersistenceOptions( - req, - projectId, - limits.compileGroup, - limits.compileBackendClass - ).catch(err => { - OError.tag(err, 'error getting cookie jar for clsi request') - throw err - }) - - const url = new URL( - action === 'output-zip-file' - ? Settings.apis.clsi.url - : Settings.apis.clsi.downloadHost - ) - url.pathname = requestPath - - const searchParams = { - ...persistenceOptions.qs, - ...qs, - } - for (const [key, value] of Object.entries(searchParams)) { - if (value !== undefined) { - // avoid sending "undefined" as a string value - url.searchParams.set(key, value) - } - } - - const timer = new Metrics.Timer( - 'proxy_to_clsi', - 1, - { path: action }, - [0, 100, 1000, 2000, 5000, 10000, 15000, 20000, 30000, 45000, 60000] - ) - Metrics.inc('proxy_to_clsi', 1, { path: action, status: 'start' }) - const ac = new AbortController() - let timeout = setTimeout(() => ac.abort(), 10_000) - try { - const { stream, response } = await fetchStreamWithResponse(url.href, { - method: req.method, - signal: ac.signal, - headers: persistenceOptions.headers, - }) - if (req.destroyed) { - // The client has disconnected already, avoid trying to write into the broken connection. - Metrics.inc('proxy_to_clsi', 1, { - path: action, - status: 'req-aborted', - }) - stream.destroy(new Error('user aborted the request')) - return - } - Metrics.inc('proxy_to_clsi', 1, { - path: action, - status: response.status, - }) - - for (const key of ['Content-Length', 'Content-Type']) { - if (response.headers.has(key)) { - res.setHeader(key, response.headers.get(key)) - } - } - - // Downloads can take a while on a slow connection, increase timeouts to 10min - const TEN_MINUTES_IN_MS = 10 * 60 * 1000 - res.setTimeout(TEN_MINUTES_IN_MS) - clearTimeout(timeout) - timeout = setTimeout(() => ac.abort(), TEN_MINUTES_IN_MS) - - // Disable buffering in nginx - res.setHeader('X-Accel-Buffering', 'no') - - res.writeHead(response.status) - await pipeline(stream, res) - timer.labels.status = 'success' - timer.done() - } catch (err) { - if (canTryClsiCacheFallback(req, res, action, err)) { - await ClsiCacheController._downloadFromCacheWithParams( - req, - res, - projectId, - `${req.query.editorId}-${req.params.build_id}`, - req.params.file - ) - return - } - const reqAborted = Boolean(req.destroyed) - const status = reqAborted ? 'req-aborted-late' : 'error' - timer.labels.status = status - const duration = timer.done() - Metrics.inc('proxy_to_clsi', 1, { path: action, status }) - const streamingStarted = Boolean(res.headersSent) - if (!streamingStarted) { - if (err instanceof RequestFailedError) { - res.sendStatus(err.response.status) - } else { - res.sendStatus(500) - } - } - if ( - streamingStarted && - reqAborted && - (err.code === 'ERR_STREAM_PREMATURE_CLOSE' || - err.code === 'ERR_STREAM_UNABLE_TO_PIPE') - ) { - // Ignore noisy spurious error - return - } - if ( - err instanceof RequestFailedError && - ['sync-to-code', 'sync-to-pdf', 'output-file'].includes(action) - ) { - // Ignore noisy error - // https://github.com/overleaf/internal/issues/15201 - return - } - logger.warn( - { - err, - projectId, - url, - action, - reqAborted, - streamingStarted, - duration, - }, - 'CLSI proxy error' - ) - } finally { - clearTimeout(timeout) - } - }, - async wordCount(req, res) { const { params, query } = parseReq(req, wordCountSchema) const projectId = params.Project_id @@ -747,41 +594,186 @@ const _CompileController = { }, } -async function _getPersistenceOptions( +async function _downloadFromClsi( + projectIdOrSubmissionId, + userId, + editorId, + buildId, + file, + clsiServerId, + action, req, - projectId, - compileGroup, - compileBackendClass + res ) { - const { clsiserverid } = req.query - const userId = SessionManager.getLoggedInUserId(req) - if (clsiserverid && typeof clsiserverid === 'string') { - return { - qs: { clsiserverid, compileGroup, compileBackendClass }, - headers: {}, - } - } else { - const clsiServerId = await ClsiCookieManager.promises.getServerId( - projectId, - userId, - compileGroup, - compileBackendClass + const { compileBackendClass } = + await CompileManager.promises.getProjectCompileLimits( + projectIdOrSubmissionId ) - return { - qs: { compileGroup, compileBackendClass }, - headers: clsiServerId - ? { - Cookie: new Cookie({ - key: Settings.clsiCookie.key, - value: clsiServerId, - }).cookieString(), - } - : {}, + const url = getOutputZipURL( + projectIdOrSubmissionId, + userId, + buildId, + compileBackendClass, + clsiServerId + ) + return await _proxyToClsi( + url, + projectIdOrSubmissionId, + userId, + editorId, + buildId, + file, + action, + req, + res + ) +} + +async function _downloadFromClsiNginx( + projectIdOrSubmissionId, + userId, + editorId, + buildId, + file, + clsiServerId, + action, + req, + res +) { + const url = getOutputFileURL( + projectIdOrSubmissionId, + userId, + buildId, + file, + clsiServerId + ) + return await _proxyToClsi( + url, + projectIdOrSubmissionId, + userId, + editorId, + buildId, + file, + action, + req, + res + ) +} + +async function _proxyToClsi( + url, + projectIdOrSubmissionId, + userId, + editorId, + buildId, + file, + action, + req, + res +) { + const timer = new Metrics.Timer( + 'proxy_to_clsi', + 1, + { path: action }, + [0, 100, 1000, 2000, 5000, 10000, 15000, 20000, 30000, 45000, 60000] + ) + Metrics.inc('proxy_to_clsi', 1, { path: action, status: 'start' }) + const ac = new AbortController() + let timeout = setTimeout(() => ac.abort(), 10_000) + try { + const { stream, response } = await fetchStreamWithResponse(url.href, { + method: req.method, + signal: ac.signal, + }) + if (req.destroyed) { + // The client has disconnected already, avoid trying to write into the broken connection. + Metrics.inc('proxy_to_clsi', 1, { + path: action, + status: 'req-aborted', + }) + stream.destroy(new Error('user aborted the request')) + return } + Metrics.inc('proxy_to_clsi', 1, { + path: action, + status: response.status, + }) + + for (const key of ['Content-Length', 'Content-Type']) { + if (response.headers.has(key)) { + res.setHeader(key, response.headers.get(key)) + } + } + + // Downloads can take a while on a slow connection, increase timeouts to 10min + const TEN_MINUTES_IN_MS = 10 * 60 * 1000 + res.setTimeout(TEN_MINUTES_IN_MS) + clearTimeout(timeout) + timeout = setTimeout(() => ac.abort(), TEN_MINUTES_IN_MS) + + // Disable buffering in nginx + res.setHeader('X-Accel-Buffering', 'no') + + res.writeHead(response.status) + await pipeline(stream, res) + timer.labels.status = 'success' + timer.done() + } catch (err) { + if (canTryClsiCacheFallback(req, res, editorId, file, action, err)) { + await ClsiCacheController._downloadFromCacheWithParams( + req, + res, + projectIdOrSubmissionId, + `${editorId}-${buildId}`, + file + ) + return + } + const reqAborted = Boolean(req.destroyed) + const status = reqAborted ? 'req-aborted-late' : 'error' + timer.labels.status = status + const duration = timer.done() + Metrics.inc('proxy_to_clsi', 1, { path: action, status }) + const streamingStarted = Boolean(res.headersSent) + if (!streamingStarted) { + if (err instanceof RequestFailedError) { + res.status(err.response.status).end() + } else { + res.status(500).end() + } + } + if ( + streamingStarted && + reqAborted && + (err.code === 'ERR_STREAM_PREMATURE_CLOSE' || + err.code === 'ERR_STREAM_UNABLE_TO_PIPE') + ) { + // Ignore noisy spurious error + return + } + if (err instanceof RequestFailedError) { + // Ignore noisy error: https://github.com/overleaf/internal/issues/15201 + return + } + logger.warn( + { + err, + projectId: projectIdOrSubmissionId, + userId, + url, + action, + reqAborted, + streamingStarted, + duration, + }, + 'CLSI proxy error' + ) + } finally { + clearTimeout(timeout) } } -function canTryClsiCacheFallback(req, res, action, err) { +function canTryClsiCacheFallback(req, res, editorId, file, action, err) { const reqAborted = Boolean(req.destroyed) const streamingStarted = Boolean(res.headersSent) return ( @@ -790,15 +782,10 @@ function canTryClsiCacheFallback(req, res, action, err) { err.response.status === 404 && !streamingStarted && !reqAborted && - req.params.build_id && - req.query.editorId && - req.params.file && - // clsi-cache only has a small subset of files available outside the tar-ball + editorId && + // clsi-cache only has a small subset of files available outside the tar-ball. // The ClsiCacheHandler will validate the filename again. - (['output.log', 'output.pdf', 'output.synctex.gz'].includes( - req.params.file - ) || - req.params.file.endsWith('.blg')) + ClsiCacheHandler.isAllowedFilename(file) ) } @@ -812,8 +799,8 @@ const CompileController = { deleteAuxFiles: expressify(_CompileController.deleteAuxFiles), getOutputZipFromClsi: expressify(_CompileController.getOutputZipFromClsi), getFileFromClsi: expressify(_CompileController.getFileFromClsi), - getFileFromClsiWithoutUser: expressify( - _CompileController.getFileFromClsiWithoutUser + getFileForSubmissionFromClsi: expressify( + _CompileController.getFileForSubmissionFromClsi ), proxySyncPdf: expressify(_CompileController.proxySyncPdf), proxySyncCode: expressify(_CompileController.proxySyncCode), @@ -822,8 +809,6 @@ const CompileController = { _getSafeProjectName: _CompileController._getSafeProjectName, _getSplitTestOptions, _getUserIdForCompile: _CompileController._getUserIdForCompile, - _proxyToClsi: _CompileController._proxyToClsi, - _proxyToClsiWithLimits: _CompileController._proxyToClsiWithLimits, } export default CompileController diff --git a/services/web/app/src/Features/LinkedFiles/ProjectOutputFileAgent.mjs b/services/web/app/src/Features/LinkedFiles/ProjectOutputFileAgent.mjs index 72fde03c02..024212dcc5 100644 --- a/services/web/app/src/Features/LinkedFiles/ProjectOutputFileAgent.mjs +++ b/services/web/app/src/Features/LinkedFiles/ProjectOutputFileAgent.mjs @@ -158,24 +158,19 @@ function _getFileStream(linkedFileData, userId, callback) { return callback(err) } const sourceProjectId = project._id - CompileManager.getProjectCompileLimits(sourceProjectId, (err, limits) => { - if (err) return callback(err) - - ClsiManager.getOutputFileStream( - sourceProjectId, - userId, - limits, - clsiServerId, - buildId, - sourceOutputFilePath, - (err, readStream) => { - if (err) { - return callback(err) - } - callback(null, readStream) + ClsiManager.getOutputFileStream( + sourceProjectId, + userId, + clsiServerId, + buildId, + sourceOutputFilePath, + (err, readStream) => { + if (err) { + return callback(err) } - ) - }) + callback(null, readStream) + } + ) }) } @@ -191,7 +186,7 @@ function _compileAndGetFileStream(linkedFileData, userId, callback) { sourceProjectId, userId, {}, - (err, status, outputFiles, clsiServerId, limits) => { + (err, status, outputFiles, clsiServerId) => { if (err) { return callback(err) } @@ -209,7 +204,6 @@ function _compileAndGetFileStream(linkedFileData, userId, callback) { ClsiManager.getOutputFileStream( sourceProjectId, userId, - limits, clsiServerId, buildId, sourceOutputFilePath, diff --git a/services/web/app/src/router.mjs b/services/web/app/src/router.mjs index 0e37ff3a2d..965ed5daa1 100644 --- a/services/web/app/src/router.mjs +++ b/services/web/app/src/router.mjs @@ -612,7 +612,7 @@ async function initialize(webRouter, privateApiRouter, publicApiRouter) { ) webRouter.get( - '/download/project/:Project_id/build/:buildId/output/cached/:filename', + '/download/project/:Project_id/build/:editorBuildId/output/cached/:filename(.*)', AuthorizationMiddleware.ensureUserCanReadProject, ClsiCacheController.downloadFromCache ) @@ -646,16 +646,7 @@ async function initialize(webRouter, privateApiRouter, publicApiRouter) { // direct url access to output files for a specific build webRouter.get( - /^\/project\/([^/]*)\/build\/([0-9a-f-]+)\/output\/(.*)$/, - function (req, res, next) { - const params = { - Project_id: req.params[0], - build_id: req.params[1], - file: req.params[2], - } - req.params = params - next() - }, + '/project/:Project_id/build/:build_id/output/:file(.*)', rateLimiterMiddlewareOutputFiles, AuthorizationMiddleware.ensureUserCanReadProject, CompileController.getFileFromClsi @@ -663,17 +654,7 @@ async function initialize(webRouter, privateApiRouter, publicApiRouter) { // direct url access to output files for a specific user and build webRouter.get( - /^\/project\/([^/]*)\/user\/([0-9a-f]+)\/build\/([0-9a-f-]+)\/output\/(.*)$/, - function (req, res, next) { - const params = { - Project_id: req.params[0], - user_id: req.params[1], - build_id: req.params[2], - file: req.params[3], - } - req.params = params - next() - }, + '/project/:Project_id/user/:user_id/build/:build_id/output/:file(.*)', rateLimiterMiddlewareOutputFiles, AuthorizationMiddleware.ensureUserCanReadProject, CompileController.getFileFromClsi diff --git a/services/web/cypress/support/shared/commands/compile.ts b/services/web/cypress/support/shared/commands/compile.ts index 44ee9c0805..152cde5261 100644 --- a/services/web/cypress/support/shared/commands/compile.ts +++ b/services/web/cypress/support/shared/commands/compile.ts @@ -1,7 +1,7 @@ import { v4 as uuid } from 'uuid' const outputFiles = () => { - const build = uuid() + const build = uuid().slice(0, 13) // first two groups of UUID return [ { @@ -200,19 +200,19 @@ export const interceptDeferredCompile = (beforeResponse?: () => void) => { outputFiles: [ { path: 'output.pdf', - build: '123', + build: '1234-5678', url: '/build/123/output.pdf', type: 'pdf', }, { path: 'output.log', - build: '123', + build: '1234-5678', url: '/build/123/output.log', type: 'log', }, { path: 'output.blg', - build: '123', + build: '1234-5678', url: '/build/123/output.blg', type: 'log', }, diff --git a/services/web/frontend/stories/fixtures/compile.js b/services/web/frontend/stories/fixtures/compile.js index e31a4488e4..b0303e1f81 100644 --- a/services/web/frontend/stories/fixtures/compile.js +++ b/services/web/frontend/stories/fixtures/compile.js @@ -10,37 +10,37 @@ export const dispatchDocChanged = () => { export const outputFiles = [ { path: 'output.pdf', - build: '123', + build: '1234-5678', url: '/build/output.pdf', type: 'pdf', }, { path: 'output.bbl', - build: '123', + build: '1234-5678', url: '/build/output.bbl', type: 'bbl', }, { path: 'output.bib', - build: '123', + build: '1234-5678', url: '/build/output.bib', type: 'bib', }, { path: 'example.txt', - build: '123', + build: '1234-5678', url: '/build/example.txt', type: 'txt', }, { path: 'output.log', - build: '123', + build: '1234-5678', url: '/build/output.log', type: 'log', }, { path: 'output.blg', - build: '123', + build: '1234-5678', url: '/build/output.blg', type: 'blg', }, diff --git a/services/web/test/acceptance/src/mocks/MockClsiApi.mjs b/services/web/test/acceptance/src/mocks/MockClsiApi.mjs index ffd065a87e..f5d4798263 100644 --- a/services/web/test/acceptance/src/mocks/MockClsiApi.mjs +++ b/services/web/test/acceptance/src/mocks/MockClsiApi.mjs @@ -8,16 +8,16 @@ class MockClsiApi extends AbstractMockApi { error: null, outputFiles: [ { - url: `http://clsi:8080/project/${req.params.project_id}/build/1234/output/output.pdf`, + url: `http://clsi:8080/project/${req.params.project_id}/build/1234-5678/output/output.pdf`, path: 'output.pdf', type: 'pdf', - build: 1234, + build: '1234-5678', }, { - url: `http://clsi:8080/project/${req.params.project_id}/build/1234/output/output.log`, + url: `http://clsi:8080/project/${req.params.project_id}/build/1234-5678/output/output.log`, path: 'output.log', type: 'log', - build: 1234, + build: '1234-5678', }, ], }, diff --git a/services/web/test/acceptance/src/mocks/MockClsiNginxApi.mjs b/services/web/test/acceptance/src/mocks/MockClsiNginxApi.mjs index 8d0b75c8ab..a07a6c8605 100644 --- a/services/web/test/acceptance/src/mocks/MockClsiNginxApi.mjs +++ b/services/web/test/acceptance/src/mocks/MockClsiNginxApi.mjs @@ -11,6 +11,8 @@ class MockClsiNginxApi extends AbstractMockApi { plainTextResponse(res, 'mock-pdf') } else if (filename === 'output.log') { plainTextResponse(res, 'mock-log') + } else if (filename.endsWith('nested.txt')) { + plainTextResponse(res, `nested.txt: ${req.originalUrl}`) } else { res.sendStatus(404) } diff --git a/services/web/test/frontend/features/file-tree/components/file-tree-create/file-tree-modal-create-file.spec.tsx b/services/web/test/frontend/features/file-tree/components/file-tree-create/file-tree-modal-create-file.spec.tsx index f2f2b69c2e..baf6042270 100644 --- a/services/web/test/frontend/features/file-tree/components/file-tree-create/file-tree-modal-create-file.spec.tsx +++ b/services/web/test/frontend/features/file-tree/components/file-tree-create/file-tree-modal-create-file.spec.tsx @@ -230,11 +230,11 @@ describe('', function () { status: 'success', outputFiles: [ { - build: 'test', + build: '1234-5678', path: 'baz.jpg', }, { - build: 'test', + build: '1234-5678', path: 'ball.jpg', }, ], @@ -299,7 +299,7 @@ describe('', function () { data: { source_project_id: 'project-2', source_output_file_path: 'ball.jpg', - build_id: 'test', + build_id: '1234-5678', }, }) }) diff --git a/services/web/test/unit/src/Compile/ClsiManager.test.mjs b/services/web/test/unit/src/Compile/ClsiManager.test.mjs index c4b0d451a7..bcdf503c77 100644 --- a/services/web/test/unit/src/Compile/ClsiManager.test.mjs +++ b/services/web/test/unit/src/Compile/ClsiManager.test.mjs @@ -289,13 +289,13 @@ describe('ClsiManager', function () { beforeEach(async function (ctx) { ctx.outputFiles = [ { - url: `/project/${ctx.project_id}/user/${ctx.user_id}/build/1234/output/output.pdf`, + url: `/project/${ctx.project_id}/user/${ctx.user_id}/build/${buildId}/output/output.pdf`, path: 'output.pdf', type: 'pdf', build: buildId, }, { - url: `/project/${ctx.project_id}/user/${ctx.user_id}/build/1234/output/output.log`, + url: `/project/${ctx.project_id}/user/${ctx.user_id}/build/${buildId}/output/output.log`, path: 'output.log', type: 'log', build: buildId, @@ -419,13 +419,13 @@ describe('ClsiManager', function () { beforeEach(async function (ctx) { ctx.outputFiles = [ { - url: `/project/${ctx.project_id}/user/${ctx.user_id}/build/1234/output/output.pdf`, + url: `/project/${ctx.project_id}/user/${ctx.user_id}/build/${buildId}/output/output.pdf`, path: 'output.pdf', type: 'pdf', build: buildId, }, { - url: `/project/${ctx.project_id}/user/${ctx.user_id}/build/1234/output/output.log`, + url: `/project/${ctx.project_id}/user/${ctx.user_id}/build/${buildId}/output/output.log`, path: 'output.log', type: 'log', build: buildId, @@ -570,20 +570,20 @@ describe('ClsiManager', function () { ctx.contentId = '123-321' ctx.outputFiles = [ { - url: `/project/${ctx.project._id}/user/${ctx.user_id}/build/1234/output/output.pdf`, + url: `/project/${ctx.project._id}/user/${ctx.user_id}/build/1234-5678/output/output.pdf`, path: 'output.pdf', type: 'pdf', - build: 1234, + build: '1234-5678', contentId: ctx.contentId, ranges: ctx.ranges, startXRefTable: ctx.startXRefTable, size: ctx.size, }, { - url: `/project/${ctx.project._id}/user/${ctx.user_id}/build/1234/output/output.log`, + url: `/project/${ctx.project._id}/user/${ctx.user_id}/build/1234-5678/output/output.log`, path: 'output.log', type: 'log', - build: 1234, + build: '1234-5678', }, ] ctx.stats = { fooStat: 1 } @@ -1097,16 +1097,16 @@ describe('ClsiManager', function () { beforeEach(async function (ctx) { ctx.outputFiles = [ { - url: `/project/${ctx.submissionId}/build/1234/output/output.pdf`, + url: `/project/${ctx.submissionId}/build/1234-5678/output/output.pdf`, path: 'output.pdf', type: 'pdf', - build: 1234, + build: '1234-5678', }, { - url: `/project/${ctx.submissionId}/build/1234/output/output.log`, + url: `/project/${ctx.submissionId}/build/1234-5678/output/output.log`, path: 'output.log', type: 'log', - build: 1234, + build: '1234-5678', }, ] ctx.responseBody.compile.outputFiles = ctx.outputFiles.map( diff --git a/services/web/test/unit/src/Compile/CompileController.test.mjs b/services/web/test/unit/src/Compile/CompileController.test.mjs index a1c70b26aa..2cf53086b1 100644 --- a/services/web/test/unit/src/Compile/CompileController.test.mjs +++ b/services/web/test/unit/src/Compile/CompileController.test.mjs @@ -5,12 +5,13 @@ import MockResponse from '../helpers/MockResponse.mjs' import { Headers } from 'node-fetch' import { ReadableString } from '@overleaf/stream-utils' import { RequestFailedError } from '@overleaf/fetch-utils' +import { asZodError } from '@overleaf/validation-tools/testUtils.js' const modulePath = '../../../../app/src/Features/Compile/CompileController.mjs' describe('CompileController', function () { beforeEach(async function (ctx) { - ctx.user_id = 'wat' + ctx.user_id = 'aaaaaaaaaaaaaaaaaaaaaaaa' ctx.user = { _id: ctx.user_id, email: 'user@example.com', @@ -25,7 +26,10 @@ describe('CompileController', function () { ctx.CompileManager = { promises: { compile: sinon.stub(), - getProjectCompileLimits: sinon.stub(), + getProjectCompileLimits: sinon.stub().resolves({ + compileBackendClass: 'c3d', + compileGroup: 'standard', + }), syncTeX: sinon.stub(), }, } @@ -92,6 +96,12 @@ describe('CompileController', function () { pipeline: ctx.pipeline, })) + ctx.Metrics = { + inc: sinon.stub(), + Timer: sinon.stub().returns({ done: sinon.stub(), labels: {} }), + } + vi.doMock('@overleaf/metrics', () => ({ default: ctx.Metrics })) + vi.doMock('@overleaf/settings', () => ({ default: ctx.settings, })) @@ -167,7 +177,10 @@ describe('CompileController', function () { ctx.CompileController = (await import(modulePath)).default ctx.projectId = 'abc123def456abc123def456' ctx.build_id = '18fbe9e7564-30dcb2f71250c690' - ctx.next = sinon.stub() + ctx.next = sinon.stub().callsFake(err => { + // Flag unexpected next calls. + throw err + }) ctx.req = new MockRequest(vi) ctx.res = new MockResponse(vi) ctx.res = new MockResponse(vi) @@ -220,7 +233,7 @@ describe('CompileController', function () { ], outputFilesArchive: { path: 'output.zip', - url: `/project/${ctx.projectId}/user/wat/build/${ctx.build_id}/output/output.zip`, + url: `/project/${ctx.projectId}/user/${ctx.user_id}/build/${ctx.build_id}/output/output.zip`, type: 'zip', }, pdfDownloadDomain: 'https://compiles.overleaf.test', @@ -265,7 +278,7 @@ describe('CompileController', function () { ], outputFilesArchive: { path: 'output.zip', - url: `/project/${ctx.projectId}/user/wat/build/${ctx.build_id}/output/output.zip`, + url: `/project/${ctx.projectId}/user/${ctx.user_id}/build/${ctx.build_id}/output/output.zip`, type: 'zip', }, outputUrlPrefix: '/zone/b', @@ -317,7 +330,7 @@ describe('CompileController', function () { outputFiles: ctx.outputFiles, outputFilesArchive: { path: 'output.zip', - url: `/project/${ctx.projectId}/user/wat/build/${ctx.build_id}/output/output.zip`, + url: `/project/${ctx.projectId}/user/${ctx.user_id}/build/${ctx.build_id}/output/output.zip`, type: 'zip', }, }) @@ -511,13 +524,18 @@ describe('CompileController', function () { describe('downloadPdf', function () { beforeEach(function (ctx) { - ctx.CompileController._proxyToClsi = sinon.stub().resolves() - ctx.req.params = { Project_id: ctx.projectId } + ctx.clsiServerId = 'clsi-server-1' + ctx.req.params = { + Project_id: ctx.projectId, + build_id: ctx.build_id, + } + ctx.req.query = { clsiserverid: ctx.clsiServerId } + ctx.req.session = {} ctx.project = { name: 'test namè; 1' } ctx.ProjectGetter.promises.getProject = sinon.stub().resolves(ctx.project) }) - describe('when downloading for embedding', function () { + describe('logged-in', function () { beforeEach(async function (ctx) { await ctx.CompileController.downloadPdf(ctx.req, ctx.res, ctx.next) }) @@ -543,36 +561,22 @@ describe('CompileController', function () { }) it('should proxy the PDF from the CLSI', function (ctx) { - ctx.CompileController._proxyToClsi - .calledWith( - ctx.projectId, - 'output-file', - `/project/${ctx.projectId}/user/${ctx.user_id}/output/output.pdf`, - {}, - ctx.req, - ctx.res - ) - .should.equal(true) + ctx.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( + `${ctx.settings.apis.clsi.downloadHost}/project/${ctx.projectId}/user/${ctx.user_id}/build/${ctx.build_id}/output/output.pdf?clsiserverid=${ctx.clsiServerId}` + ) }) }) - describe('when a build-id is provided', function () { + describe('anon', function () { beforeEach(async function (ctx) { - ctx.req.params.build_id = ctx.build_id + ctx.SessionManager.getLoggedInUserId.returns(null) await ctx.CompileController.downloadPdf(ctx.req, ctx.res, ctx.next) }) - it('should proxy the PDF from the CLSI, with a build-id', function (ctx) { - ctx.CompileController._proxyToClsi - .calledWith( - ctx.projectId, - 'output-file', - `/project/${ctx.projectId}/user/${ctx.user_id}/build/${ctx.build_id}/output/output.pdf`, - {}, - ctx.req, - ctx.res - ) - .should.equal(true) + it('should proxy the PDF from the CLSI', function (ctx) { + ctx.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( + `${ctx.settings.apis.clsi.downloadHost}/project/${ctx.projectId}/build/${ctx.build_id}/output/output.pdf?clsiserverid=${ctx.clsiServerId}` + ) }) }) @@ -587,9 +591,8 @@ describe('CompileController', function () { }) it('should return 500', async function (ctx) { await ctx.CompileController.downloadPdf(ctx.req, ctx.res, ctx.next) - // should it be 429 instead? - expect(ctx.res.sendStatus).toBeCalledWith(500) - ctx.CompileController._proxyToClsi.should.not.have.been.called + expect(ctx.res.status).toBeCalledWith(429) + ctx.fetchUtils.fetchStreamWithResponse.should.not.have.been.called }) }) @@ -599,70 +602,111 @@ describe('CompileController', function () { }) it('should return 500', async function (ctx) { await ctx.CompileController.downloadPdf(ctx.req, ctx.res, ctx.next) - expect(ctx.res.sendStatus).toBeCalledWith(500) - ctx.CompileController._proxyToClsi.should.not.have.been.called + expect(ctx.res.status).toBeCalledWith(500) + ctx.fetchUtils.fetchStreamWithResponse.should.not.have.been.called }) }) }) - describe('getFileFromClsiWithoutUser', function () { + describe('getOutputZipFromClsi', function () { + beforeEach(function (ctx) { + ctx.clsiServerId = 'clsi-server-1' + ctx.req.params = { + Project_id: ctx.projectId, + build_id: ctx.build_id, + } + ctx.req.query = { clsiserverid: ctx.clsiServerId } + ctx.req.session = {} + ctx.project = { name: 'test namè; 1' } + ctx.ProjectGetter.promises.getProject = sinon.stub().resolves(ctx.project) + }) + + describe('free user', function () { + beforeEach(async function (ctx) { + await ctx.CompileController.getOutputZipFromClsi( + ctx.req, + ctx.res, + ctx.next + ) + }) + + it('should look up the project', function (ctx) { + ctx.ProjectGetter.promises.getProject + .calledWith(ctx.projectId, { name: 1 }) + .should.equal(true) + }) + + it('should set the content-type of the response to application/zip', function (ctx) { + expect(ctx.res.contentType).toBeCalledWith('application/zip') + }) + + it('should set the content-disposition header with a safe version of the project name', function (ctx) { + expect(ctx.res.headers['Content-Disposition']).toEqual( + 'attachment; filename="test_namè__1-output.zip"' + ) + }) + + it('should proxy the PDF from the CLSI', function (ctx) { + ctx.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( + `${ctx.settings.apis.clsi.url}/project/${ctx.projectId}/user/${ctx.user_id}/build/${ctx.build_id}/output/output.zip?compileBackendClass=c3d&clsiserverid=${ctx.clsiServerId}` + ) + }) + }) + + describe('premium user', function () { + beforeEach(async function (ctx) { + ctx.CompileManager.promises.getProjectCompileLimits = sinon + .stub() + .resolves({ + compileGroup: 'priority', + compileBackendClass: 'c4d', + }) + await ctx.CompileController.getOutputZipFromClsi( + ctx.req, + ctx.res, + ctx.next + ) + }) + + it('should proxy the PDF from the CLSI', function (ctx) { + ctx.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( + `${ctx.settings.apis.clsi.url}/project/${ctx.projectId}/user/${ctx.user_id}/build/${ctx.build_id}/output/output.zip?compileBackendClass=c4d&clsiserverid=${ctx.clsiServerId}` + ) + }) + }) + }) + + describe('getFileForSubmissionFromClsi', function () { beforeEach(function (ctx) { ctx.submission_id = 'sub-1234' + ctx.clsiServerId = 'clsi-server-1' ctx.file = 'output.pdf' ctx.req.params = { - submission_id: ctx.submission_id, + submissionId: ctx.submission_id, build_id: ctx.build_id, file: ctx.file, } - ctx.req.body = {} - ctx.expected_url = `/project/${ctx.submission_id}/build/${ctx.build_id}/output/${ctx.file}` - ctx.CompileController._proxyToClsiWithLimits = sinon.stub() + ctx.req.query = { clsiserverid: ctx.clsiServerId } }) - describe('without limits specified', function () { + describe('proxy to CLSI with correct URL', function () { beforeEach(async function (ctx) { - await ctx.CompileController.getFileFromClsiWithoutUser( + await ctx.CompileController.getFileForSubmissionFromClsi( ctx.req, ctx.res, ctx.next ) }) - it('should proxy to CLSI with correct URL and default limits', function (ctx) { - ctx.CompileController._proxyToClsiWithLimits.should.have.been.calledWith( - ctx.submission_id, - 'output-file', - ctx.expected_url, - {}, - { compileGroup: 'standard', compileBackendClass: 'c3d' } - ) - }) - }) - - describe('with limits specified', function () { - beforeEach(function (ctx) { - ctx.req.body = { compileTimeout: 600, compileGroup: 'special' } - ctx.CompileController.getFileFromClsiWithoutUser( - ctx.req, - ctx.res, - ctx.next - ) - }) - - it('should proxy to CLSI with correct URL and specified limits', function (ctx) { - ctx.CompileController._proxyToClsiWithLimits.should.have.been.calledWith( - ctx.submission_id, - 'output-file', - ctx.expected_url, - {}, - { - compileGroup: 'special', - compileBackendClass: 'c3d', - } + it('should proxy to CLSI with correct URL', function (ctx) { + const expectedUrl = `${ctx.settings.apis.clsi.downloadHost}/project/${ctx.submission_id}/build/${ctx.build_id}/output/${ctx.file}?clsiserverid=${ctx.clsiServerId}` + ctx.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( + expectedUrl ) }) }) }) + describe('proxySyncCode', function () { let file, line, column, imageName, editorId, buildId, clsiServerId @@ -763,248 +807,148 @@ describe('CompileController', function () { }) }) - describe('_proxyToClsi', function () { + describe('getFileFromClsi', function () { beforeEach(function (ctx) { - ctx.req.method = 'mock-method' - ctx.req.headers = { - Mock: 'Headers', - Range: '123-456', - 'If-Range': 'abcdef', - 'If-Modified-Since': 'Mon, 15 Dec 2014 15:23:56 GMT', + ctx.clsiServerId = 'clsi-server-1' + ctx.req.params = { + Project_id: ctx.projectId, + build_id: ctx.build_id, + file: 'output.blg', } + ctx.req.query = { clsiserverid: ctx.clsiServerId } + ctx.req.session = {} + ctx.req.method = 'GET' }) - describe('old pdf viewer', function () { - describe('user with standard priority', function () { - beforeEach(async function (ctx) { - ctx.CompileManager.promises.getProjectCompileLimits = sinon - .stub() - .resolves({ - compileGroup: 'standard', - compileBackendClass: 'c3d', - }) - await ctx.CompileController._proxyToClsi( - ctx.projectId, - 'output-file', - (ctx.url = '/test'), - { query: 'foo' }, - ctx.req, - ctx.res, - ctx.next - ) - }) + describe('when the output.blg exists', function () { + beforeEach(async function (ctx) { + await ctx.CompileController.getFileFromClsi(ctx.req, ctx.res, ctx.next) + }) - it('should open a request to the CLSI', function (ctx) { - ctx.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( - `${ctx.settings.apis.clsi.downloadHost}${ctx.url}?compileGroup=standard&compileBackendClass=c3d&query=foo` - ) - }) + it('should open a request to the CLSI download host with compile limits', function (ctx) { + ctx.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( + `${ctx.settings.apis.clsi.downloadHost}/project/${ctx.projectId}/user/${ctx.user_id}/build/${ctx.build_id}/output/output.blg?clsiserverid=${ctx.clsiServerId}` + ) + }) - it('should pass the request on to the client', function (ctx) { - ctx.pipeline.should.have.been.calledWith(ctx.clsiStream, ctx.res) + it('should pass the response stream on to the client', function (ctx) { + ctx.pipeline.should.have.been.calledWith(ctx.clsiStream, ctx.res) + }) + }) + + describe('when the output.blg traverses up', function () { + beforeEach(async function (ctx) { + ctx.req.params.file = '../output.blg' + ctx.next = sinon.stub() + await ctx.CompileController.getFileFromClsi(ctx.req, ctx.res, ctx.next) + }) + + it('should reject the request', function (ctx) { + ctx.next.should.have.been.calledWithMatch({ + name: 'ParamsError', + cause: asZodError({ + code: 'custom', + path: ['params', 'file'], + message: 'path traversal detected', + }), }) }) - describe('user with priority compile', function () { - beforeEach(async function (ctx) { - ctx.CompileManager.promises.getProjectCompileLimits = sinon - .stub() - .resolves({ - compileGroup: 'priority', - compileBackendClass: 'c4d', - }) - await ctx.CompileController._proxyToClsi( - ctx.projectId, - 'output-file', - (ctx.url = '/test'), - {}, - ctx.req, - ctx.res, - ctx.next - ) - }) + it('should not open a request to CLSI', function (ctx) { + ctx.fetchUtils.fetchStreamWithResponse.should.not.have.been.called + }) + }) - it('should open a request to the CLSI', function (ctx) { - ctx.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( - `${ctx.settings.apis.clsi.downloadHost}${ctx.url}?compileGroup=priority&compileBackendClass=c4d` - ) + describe('when the buildId traverses up', function () { + beforeEach(async function (ctx) { + ctx.req.params.build_id = '../..' + ctx.next = sinon.stub() + await ctx.CompileController.getFileFromClsi(ctx.req, ctx.res, ctx.next) + }) + + it('should reject the request', function (ctx) { + ctx.next.should.have.been.calledWithMatch({ + name: 'ParamsError', + cause: asZodError({ + origin: 'string', + code: 'invalid_format', + format: 'regex', + pattern: '/^[0-9a-f]+-[0-9a-f]+$/', + path: ['params', 'build_id'], + message: 'invalid buildId', + }), }) }) - describe('when the output.pdf does not exist', function () { - beforeEach(async function (ctx) { - ctx.req.params.file = 'output.pdf' - ctx.req.params.build_id = ctx.build_id - ctx.url = `/project/${ctx.projectId}/build/${ctx.build_id}/output/${ctx.req.params.file}` - ctx.editorId = '00000000-0000-0000-0000-000000000042' - ctx.req.query = { editorId: ctx.editorId } - ctx.CompileManager.promises.getProjectCompileLimits = sinon - .stub() - .resolves({ - compileGroup: 'priority', - compileBackendClass: 'c4d', - }) - ctx.fetchUtils.fetchStreamWithResponse.rejects( - new RequestFailedError( - `${ctx.settings.apis.clsi.downloadHost}${ctx.url}?compileGroup=priority&compileBackendClass=c4d`, - { method: 'GET' }, - { status: 404 } - ) - ) - await ctx.CompileController._proxyToClsi( - ctx.projectId, - 'output-file', - ctx.url, - {}, - ctx.req, - ctx.res, - ctx.next - ) - }) - - it('should open a request to the CLSI', function (ctx) { - ctx.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( - `${ctx.settings.apis.clsi.downloadHost}${ctx.url}?compileGroup=priority&compileBackendClass=c4d` - ) - }) - - it('should fallback to clsi-cache', function (ctx) { - ctx.ClsiCacheController._downloadFromCacheWithParams.should.have.been.calledWith( - ctx.req, - ctx.res, - ctx.projectId, - `${ctx.editorId}-${ctx.build_id}`, - 'output.pdf' - ) - }) + it('should not open a request to CLSI', function (ctx) { + ctx.fetchUtils.fetchStreamWithResponse.should.not.have.been.called }) - describe('when the output.stderr does not exist', function () { - beforeEach(async function (ctx) { - ctx.req.params.file = 'output.stderr' - ctx.req.params.build_id = ctx.build_id - ctx.url = `/project/${ctx.projectId}/build/${ctx.build_id}/output/${ctx.req.params.file}` - ctx.editorId = '00000000-0000-0000-0000-000000000042' - ctx.req.query = { editorId: ctx.editorId } - ctx.CompileManager.promises.getProjectCompileLimits = sinon - .stub() - .resolves({ - compileGroup: 'priority', - compileBackendClass: 'c4d', - }) - ctx.fetchUtils.fetchStreamWithResponse.rejects( - new RequestFailedError( - `${ctx.settings.apis.clsi.downloadHost}${ctx.url}?compileGroup=priority&compileBackendClass=c4d`, - { method: 'GET' }, - { status: 404 } - ) - ) - await ctx.CompileController._proxyToClsi( - ctx.projectId, - 'output-file', - ctx.url, - {}, - ctx.req, - ctx.res, - ctx.next - ) - }) + }) - it('should open a request to the CLSI', function (ctx) { - ctx.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( - `${ctx.settings.apis.clsi.downloadHost}${ctx.url}?compileGroup=priority&compileBackendClass=c4d` + describe('when the output.blg does not exist', function () { + beforeEach(async function (ctx) { + ctx.editorId = '0e546f78-928e-4e8a-b5ea-3136ccf1dc53' + ctx.req.query = { + clsiserverid: ctx.clsiServerId, + editorId: ctx.editorId, + } + ctx.clsiURL = `${ctx.settings.apis.clsi.downloadHost}/project/${ctx.projectId}/user/${ctx.user_id}/build/${ctx.build_id}/output/output.blg?clsiserverid=${ctx.clsiServerId}` + ctx.fetchUtils.fetchStreamWithResponse.rejects( + new RequestFailedError( + ctx.clsiURL, + { method: 'GET' }, + { status: 404 } ) - }) - - it('should not fallback to clsi-cache', function (ctx) { - ctx.ClsiCacheController._downloadFromCacheWithParams.should.not.have - .been.called - ctx.res.statusCode.should.equal(404) - }) + ) + await ctx.CompileController.getFileFromClsi(ctx.req, ctx.res, ctx.next) }) - describe('user with standard priority via query string', function () { - beforeEach(async function (ctx) { - ctx.req.query = { compileGroup: 'standard' } - ctx.CompileManager.promises.getProjectCompileLimits = sinon - .stub() - .resolves({ - compileGroup: 'standard', - compileBackendClass: 'c3d', - }) - await ctx.CompileController._proxyToClsi( - ctx.projectId, - 'output-file', - (ctx.url = '/test'), - {}, - ctx.req, - ctx.res, - ctx.next - ) - }) - - it('should open a request to the CLSI', function (ctx) { - ctx.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( - `${ctx.settings.apis.clsi.downloadHost}${ctx.url}?compileGroup=standard&compileBackendClass=c3d` - ) - }) - - it('should pass the request on to the client', function (ctx) { - ctx.pipeline.should.have.been.calledWith(ctx.clsiStream, ctx.res) - }) + it('should open a request to the CLSI', function (ctx) { + ctx.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( + ctx.clsiURL + ) }) - describe('user with non-existent priority via query string', function () { - beforeEach(async function (ctx) { - ctx.req.query = { compileGroup: 'foobar' } - ctx.CompileManager.promises.getProjectCompileLimits = sinon - .stub() - .resolves({ - compileGroup: 'standard', - compileBackendClass: 'c3d', - }) - await ctx.CompileController._proxyToClsi( - ctx.projectId, - 'output-file', - (ctx.url = '/test'), - {}, - ctx.req, - ctx.res, - ctx.next - ) - }) + it('should fallback to clsi-cache', function (ctx) { + ctx.ClsiCacheController._downloadFromCacheWithParams.should.have.been.calledWith( + ctx.req, + ctx.res, + ctx.projectId, + `${ctx.editorId}-${ctx.build_id}`, + 'output.blg' + ) + }) + }) - it('should proxy to the standard url', function (ctx) { - ctx.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( - `${ctx.settings.apis.clsi.downloadHost}${ctx.url}?compileGroup=standard&compileBackendClass=c3d` + describe('when the output.stderr does not exist', function () { + beforeEach(async function (ctx) { + ctx.req.params.file = 'output.stderr' + ctx.editorId = '0e546f78-928e-4e8a-b5ea-3136ccf1dc53' + ctx.req.query = { + clsiserverid: ctx.clsiServerId, + editorId: ctx.editorId, + } + ctx.clsiURL = `${ctx.settings.apis.clsi.downloadHost}/project/${ctx.projectId}/user/${ctx.user_id}/build/${ctx.build_id}/output/output.stderr?clsiserverid=${ctx.clsiServerId}` + ctx.fetchUtils.fetchStreamWithResponse.rejects( + new RequestFailedError( + ctx.clsiURL, + { method: 'GET' }, + { status: 404 } ) - }) + ) + await ctx.CompileController.getFileFromClsi(ctx.req, ctx.res, ctx.next) }) - describe('user with build parameter via query string', function () { - beforeEach(async function (ctx) { - ctx.CompileManager.promises.getProjectCompileLimits = sinon - .stub() - .resolves({ - compileGroup: 'standard', - compileBackendClass: 'c3d', - }) - ctx.req.query = { build: 1234 } - await ctx.CompileController._proxyToClsi( - ctx.projectId, - 'output-file', - (ctx.url = '/test'), - {}, - ctx.req, - ctx.res, - ctx.next - ) - }) + it('should open a request to the CLSI', function (ctx) { + ctx.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( + ctx.clsiURL + ) + }) - it('should proxy to the standard url without the build parameter', function (ctx) { - ctx.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( - `${ctx.settings.apis.clsi.downloadHost}${ctx.url}?compileGroup=standard&compileBackendClass=c3d` - ) - }) + it('should not fallback to clsi-cache', function (ctx) { + ctx.ClsiCacheController._downloadFromCacheWithParams.should.not.have + .been.called + expect(ctx.res.statusCode).to.equal(404) }) }) }) @@ -1030,21 +974,28 @@ describe('CompileController', function () { }) describe('compileAndDownloadPdf', function () { + const clsiServerId = 'server-1' + beforeEach(function (ctx) { ctx.req = { params: { project_id: ctx.projectId, }, + method: 'GET', } - ctx.downloadPath = `/project/${ctx.projectId}/build/123/output/output.pdf` ctx.CompileManager.promises.compile.resolves({ status: 'success', - outputFiles: [{ path: 'output.pdf', url: ctx.downloadPath }], + outputFiles: [{ path: 'output.pdf' }], + clsiServerId, + buildId: ctx.build_id, }) - ctx.CompileController._proxyToClsi = sinon.stub() ctx.res = { send: () => {}, sendStatus: sinon.stub(), + writeHead: sinon.stub(), + setHeader: sinon.stub(), + setTimeout: sinon.stub(), + headersSent: false, } }) @@ -1055,28 +1006,11 @@ describe('CompileController', function () { .should.equal(true) }) - it('should proxy the res to the clsi with correct url', async function (ctx) { + it('should proxy the PDF from the CLSI with the correct URL', async function (ctx) { await ctx.CompileController.compileAndDownloadPdf(ctx.req, ctx.res) - sinon.assert.calledWith( - ctx.CompileController._proxyToClsi, - ctx.projectId, - 'output-file', - ctx.downloadPath, - {}, - ctx.req, - ctx.res + ctx.fetchUtils.fetchStreamWithResponse.should.have.been.calledWith( + `${ctx.settings.apis.clsi.downloadHost}/project/${ctx.projectId}/build/${ctx.build_id}/output/output.pdf?clsiserverid=${clsiServerId}` ) - - ctx.CompileController._proxyToClsi - .calledWith( - ctx.projectId, - 'output-file', - ctx.downloadPath, - {}, - ctx.req, - ctx.res - ) - .should.equal(true) }) it('should not download anything on compilation failures', async function (ctx) { @@ -1087,17 +1021,19 @@ describe('CompileController', function () { ctx.next ) ctx.res.sendStatus.should.have.been.calledWith(500) - ctx.CompileController._proxyToClsi.should.not.have.been.called + ctx.fetchUtils.fetchStreamWithResponse.should.not.have.been.called }) it('should not download anything on missing pdf', async function (ctx) { ctx.CompileManager.promises.compile.resolves({ status: 'success', outputFiles: [], + clsiServerId, + buildId: ctx.build_id, }) await ctx.CompileController.compileAndDownloadPdf(ctx.req, ctx.res) ctx.res.sendStatus.should.have.been.calledWith(500) - ctx.CompileController._proxyToClsi.should.not.have.been.called + ctx.fetchUtils.fetchStreamWithResponse.should.not.have.been.called }) })