diff --git a/libraries/validation-tools/Errors.js b/libraries/validation-tools/Errors.js index 197289801f..e6a12d4e54 100644 --- a/libraries/validation-tools/Errors.js +++ b/libraries/validation-tools/Errors.js @@ -1,29 +1,5 @@ -// @ts-check - const OError = require('@overleaf/o-error') -/** - * @typedef {import('zod').ZodError} ZodError - */ +class ParamsError extends OError {} -class InvalidRequestError extends OError { - /** - * @param {ZodError} zodError - */ - constructor(zodError) { - super('Invalid request', {}, zodError) - this.zodError = zodError - } -} - -class InvalidParamsError extends OError { - /** - * @param {ZodError} zodError - */ - constructor(zodError) { - super('Invalid request parameters', {}, zodError) - this.zodError = zodError - } -} - -module.exports = { InvalidParamsError, InvalidRequestError } +module.exports = { ParamsError } diff --git a/libraries/validation-tools/handleValidationError.js b/libraries/validation-tools/handleValidationError.js index f7b24528e5..a5038448eb 100644 --- a/libraries/validation-tools/handleValidationError.js +++ b/libraries/validation-tools/handleValidationError.js @@ -1,20 +1,15 @@ -const { fromError } = require('zod-validation-error') -const { InvalidParamsError, InvalidRequestError } = require('./Errors') +const { isZodErrorLike, fromError } = require('zod-validation-error') function createHandleValidationError(statusCode = 400) { - return (err, req, res, next) => { - if (err instanceof InvalidParamsError) { - res - .status(404) - .json({ error: fromError(err.zodError).toString(), statusCode: 404 }) - } else if (err instanceof InvalidRequestError) { - res - .status(statusCode) - .json({ error: fromError(err.zodError).toString(), statusCode }) - } else { - next(err) - } - } + return [ + (err, req, res, next) => { + if (!isZodErrorLike(err)) { + return next(err) + } + + res.status(statusCode).json({ ...fromError(err), statusCode }) + }, + ] } const handleValidationError = createHandleValidationError(400) diff --git a/libraries/validation-tools/index.js b/libraries/validation-tools/index.js index 6a4169117f..c456a3a7bf 100644 --- a/libraries/validation-tools/index.js +++ b/libraries/validation-tools/index.js @@ -1,4 +1,4 @@ -const { InvalidParamsError, InvalidRequestError } = require('./Errors') +const { ParamsError } = require('./Errors') const { z } = require('zod') const { zz } = require('./zodHelpers') const { parseReq } = require('./parseReq') @@ -15,6 +15,5 @@ module.exports = { parseReq, handleValidationError, createHandleValidationError, - InvalidRequestError, - InvalidParamsError, + ParamsError, } diff --git a/libraries/validation-tools/parseReq.js b/libraries/validation-tools/parseReq.js index 28818a4eb8..cb3c87c659 100644 --- a/libraries/validation-tools/parseReq.js +++ b/libraries/validation-tools/parseReq.js @@ -1,5 +1,5 @@ // @ts-check -const { InvalidRequestError, InvalidParamsError } = require('./Errors') +const { ParamsError } = require('./Errors') /** * @typedef {import('zod').ZodType} ZodType @@ -25,9 +25,9 @@ function parseReq(req, schema) { return parsed.data } else if (parsed.error.issues.some(issue => issue.path[0] === 'params')) { // Parts of the URL path failed to validate; throw a specific error - throw new InvalidParamsError(parsed.error) + throw new ParamsError('Invalid params').withCause(parsed.error) } else { - throw new InvalidRequestError(parsed.error) + throw parsed.error } } diff --git a/libraries/validation-tools/test/unit/src/validateReq.test.ts b/libraries/validation-tools/test/unit/src/validateReq.test.ts index 69bb516458..a30e14c299 100644 --- a/libraries/validation-tools/test/unit/src/validateReq.test.ts +++ b/libraries/validation-tools/test/unit/src/validateReq.test.ts @@ -54,7 +54,7 @@ describe('parseReq', () => { }), }) ) - ).toThrowError(expect.objectContaining({ name: 'InvalidParamsError' })) + ).toThrowError(expect.objectContaining({ name: 'ParamsError' })) }) it('should throw an error containing issues if the schema is invalid', () => { @@ -75,11 +75,9 @@ describe('parseReq', () => { ) ).toThrowError( expect.objectContaining({ - zodError: expect.objectContaining({ - issues: expect.arrayContaining([ - expect.objectContaining({ path: ['body', 'name'] }), - ]), - }), + issues: expect.arrayContaining([ + expect.objectContaining({ path: ['body', 'name'] }), + ]), }) ) }) diff --git a/server-ce/test/host-admin.js b/server-ce/test/host-admin.js index a56cb218a1..c871c6ed23 100644 --- a/server-ce/test/host-admin.js +++ b/server-ce/test/host-admin.js @@ -5,12 +5,8 @@ import { execFile as execFileCb } from 'node:child_process' import bodyParser from 'body-parser' import express from 'express' import YAML from 'js-yaml' -import { - InvalidParamsError, - InvalidRequestError, - parseReq, - z, -} from '@overleaf/validation-tools' +import { isZodErrorLike } from 'zod-validation-error' +import { ParamsError, parseReq, z } from '@overleaf/validation-tools' import { expressify } from '@overleaf/promise-utils' const execFile = promisify(execFileCb) @@ -478,13 +474,12 @@ app.delete( ) app.use((error, req, res, next) => { - if (error instanceof InvalidParamsError) { + if (error instanceof ParamsError) { res.status(404).json({ error }) - } else if (error instanceof InvalidRequestError) { - res.status(400).json({ error: error.zodError }) - } else { - next(error) + } else if (isZodErrorLike(error)) { + res.status(400).json({ error }) } + next(error) }) purgeDataDir() diff --git a/services/notifications/app.ts b/services/notifications/app.ts index 3232e24be1..71e5a12017 100644 --- a/services/notifications/app.ts +++ b/services/notifications/app.ts @@ -13,10 +13,8 @@ import methodOverride from 'method-override' import { mongoClient } from './app/js/mongodb.js' import NotificationsController from './app/js/NotificationsController.ts' import HealthCheckController from './app/js/HealthCheckController.ts' -import { - InvalidParamsError, - InvalidRequestError, -} from '@overleaf/validation-tools' +import { isZodErrorLike } from 'zod-validation-error' +import { ParamsError } from '@overleaf/validation-tools' const app = express() @@ -58,10 +56,10 @@ const handleApiError: ErrorRequestHandler = ( next: NextFunction ) => { req.logger.addFields({ err }) - if (err instanceof InvalidParamsError) { + if (err instanceof ParamsError) { req.logger.setLevel('warn') res.sendStatus(404) - } else if (err instanceof InvalidRequestError) { + } else if (isZodErrorLike(err)) { req.logger.setLevel('warn') res.sendStatus(400) } else { diff --git a/services/real-time/app/js/Router.js b/services/real-time/app/js/Router.js index 794939da4e..e29873fcbf 100644 --- a/services/real-time/app/js/Router.js +++ b/services/real-time/app/js/Router.js @@ -40,7 +40,7 @@ export default Router = { attrs.client_id = client.id attrs.err = error attrs.method = method - if (attrs.validation && isZodErrorLike(error)) { + if (isZodErrorLike(error)) { logger.info(attrs, 'validation error') let message = 'invalid' try { @@ -456,7 +456,6 @@ export default Router = { joinDocSchema.parse({ doc_id: docId, fromVersion, options }) } catch (error) { return Router._handleError(callback, error, client, 'joinDoc', { - validation: 1, disconnect: 1, }) } @@ -486,7 +485,6 @@ export default Router = { zz.objectId().parse(docId) } catch (error) { return Router._handleError(callback, error, client, 'joinDoc', { - validation: 1, disconnect: 1, }) } @@ -572,7 +570,6 @@ export default Router = { applyOtUpdateSchema.parse({ doc_id: docId, update }) } catch (error) { return Router._handleError(callback, error, client, 'applyOtUpdate', { - validation: 1, disconnect: 1, }) } diff --git a/services/web/app/src/Features/Errors/ErrorController.mjs b/services/web/app/src/Features/Errors/ErrorController.mjs index 37316304a8..b70774b2a6 100644 --- a/services/web/app/src/Features/Errors/ErrorController.mjs +++ b/services/web/app/src/Features/Errors/ErrorController.mjs @@ -1,14 +1,11 @@ -import { fromZodError } from 'zod-validation-error' -import { - InvalidRequestError, - InvalidParamsError, -} from '@overleaf/validation-tools' +import { isZodErrorLike, fromZodError } from 'zod-validation-error' import Errors, { NotFoundError } from './Errors.js' import SessionManager from '../Authentication/SessionManager.mjs' import SamlLogHandler from '../SamlLog/SamlLogHandler.mjs' import HttpErrorHandler from './HttpErrorHandler.mjs' import { plainTextResponse } from '../../infrastructure/Response.mjs' import { expressifyErrorHandler } from '@overleaf/promise-utils' +import { ParamsError } from '@overleaf/validation-tools' function notFound(req, res) { res.status(404) @@ -45,7 +42,7 @@ async function handleError(error, req, res, next) { if (shouldSendErrorResponse) { notFound(req, res) } - } else if (error instanceof InvalidParamsError) { + } else if (error instanceof ParamsError) { req.logger.setLevel('warn') if (shouldSendErrorResponse) { notFound(req, res) @@ -107,11 +104,11 @@ async function handleError(error, req, res, next) { res.status(400) plainTextResponse(res, error.message) } - } else if (error instanceof InvalidRequestError) { + } else if (isZodErrorLike(error)) { req.logger.setLevel('warn') res.status(400) if (shouldSendErrorResponse) { - const validationError = fromZodError(error.zodError) + const validationError = fromZodError(error) res.render('general/400', { message: validationError.message }) } } else { @@ -129,10 +126,7 @@ async function handleError(error, req, res, next) { function handleApiError(err, req, res, next) { req.logger.addFields({ err }) - if ( - err instanceof Errors.NotFoundError || - err instanceof InvalidParamsError - ) { + if (err instanceof Errors.NotFoundError || err instanceof ParamsError) { req.logger.setLevel('warn') res.sendStatus(404) } else if ( @@ -147,7 +141,7 @@ function handleApiError(err, req, res, next) { } else if (err instanceof Errors.ForbiddenError) { req.logger.setLevel('warn') res.sendStatus(403) - } else if (err instanceof InvalidRequestError) { + } else if (isZodErrorLike(err)) { req.logger.setLevel('warn') res.sendStatus(400) } else { diff --git a/services/web/app/src/Features/Security/LoginRateLimiter.mjs b/services/web/app/src/Features/Security/LoginRateLimiter.mjs index 276904123f..4fb032f222 100644 --- a/services/web/app/src/Features/Security/LoginRateLimiter.mjs +++ b/services/web/app/src/Features/Security/LoginRateLimiter.mjs @@ -2,7 +2,6 @@ import { RateLimiter } from '../../infrastructure/RateLimiter.mjs' import { callbackify } from '@overleaf/promise-utils' import Settings from '@overleaf/settings' import EmailHelper from '../Helpers/EmailHelper.mjs' -import { InvalidRequestError } from '@overleaf/validation-tools' const rateLimiterLoginEmail = new RateLimiter( 'login', @@ -13,11 +12,7 @@ const rateLimiterLoginEmail = new RateLimiter( ) async function processLoginRequest(email) { - try { - email = EmailHelper.emailSchema.parse(email) - } catch (err) { - throw new InvalidRequestError(err) - } + email = EmailHelper.emailSchema.parse(email) try { await rateLimiterLoginEmail.consume(email, 1, { method: 'email', diff --git a/services/web/app/src/infrastructure/Validation.mjs b/services/web/app/src/infrastructure/Validation.mjs index 76a8a7cc39..41349520bb 100644 --- a/services/web/app/src/infrastructure/Validation.mjs +++ b/services/web/app/src/infrastructure/Validation.mjs @@ -2,7 +2,7 @@ import { parseReq, z, zz } from '@overleaf/validation-tools' -export { InvalidParamsError, parseReq, z, zz } from '@overleaf/validation-tools' +export { ParamsError, parseReq, z, zz } from '@overleaf/validation-tools' export default { parseReq, diff --git a/services/web/test/acceptance/src/AuthenticationTests.mjs b/services/web/test/acceptance/src/AuthenticationTests.mjs index 16f8c1a704..d3acb19de3 100644 --- a/services/web/test/acceptance/src/AuthenticationTests.mjs +++ b/services/web/test/acceptance/src/AuthenticationTests.mjs @@ -176,12 +176,31 @@ describe('Authentication', function () { } }) it('should return 400 with bad email (missing @)', async function () { - const { statusCode } = await tryLoginWithEmail('foo') + const { statusCode, body } = await tryLoginWithEmail('foo') expect(statusCode).to.equal(400) + expect(body).to.deep.equal({ + name: 'ZodValidationError', + details: [ + { code: 'custom', path: [], message: 'Invalid email address' }, + ], + statusCode: 400, + }) }) it('should return 400 with bad email (number)', async function () { - const { statusCode } = await tryLoginWithEmail(1) + const { statusCode, body } = await tryLoginWithEmail(1) expect(statusCode).to.equal(400) + expect(body).to.deep.equal({ + name: 'ZodValidationError', + details: [ + { + expected: 'string', + code: 'invalid_type', + path: [], + message: 'Invalid input: expected string, received number', + }, + ], + statusCode: 400, + }) }) }) }) diff --git a/services/web/test/acceptance/src/ProjectCRUDTests.mjs b/services/web/test/acceptance/src/ProjectCRUDTests.mjs index 0a95d254b3..af711f29be 100644 --- a/services/web/test/acceptance/src/ProjectCRUDTests.mjs +++ b/services/web/test/acceptance/src/ProjectCRUDTests.mjs @@ -219,13 +219,14 @@ describe('Project CRUD', function () { }) it('returns a 400 when publicAccessLevel is an unsupported access level', async function () { await this.user.makePrivate(this.projectId) - const { response } = await this.user.doRequest('POST', { + const { response, body } = await this.user.doRequest('POST', { url: `/project/${this.projectId}/settings/admin`, json: { publicAccessLevel: 'readOnly', }, }) expect(response.statusCode).to.equal(400) + expect(body.details[0].message).to.equal('unexpected access level') const project = await Project.findById(this.projectId).exec() expect(project.publicAccesLevel).to.equal('private') }) diff --git a/services/web/test/acceptance/src/ProjectInviteTests.mjs b/services/web/test/acceptance/src/ProjectInviteTests.mjs index 926d9dcfdc..525a1bbc82 100644 --- a/services/web/test/acceptance/src/ProjectInviteTests.mjs +++ b/services/web/test/acceptance/src/ProjectInviteTests.mjs @@ -375,6 +375,11 @@ describe('ProjectInviteTests', function () { return done(err) } expect(response.statusCode).to.equal(400) + expect(body.details).to.have.lengthOf(1) + expect(response.body.details[0].path).to.eql(['body', 'email']) + expect(response.body.details[0].message).to.equal( + 'Invalid input: expected string, received object' + ) done() } ) @@ -399,6 +404,14 @@ describe('ProjectInviteTests', function () { return done(err) } expect(response.statusCode).to.equal(400) + expect(body.details).to.have.lengthOf(1) + expect(response.body.details[0].path).to.eql([ + 'body', + 'privileges', + ]) + expect(response.body.details[0].message).to.equal( + 'Invalid option: expected one of "readOnly"|"readAndWrite"|"review"' + ) done() } ) diff --git a/services/web/test/unit/src/Subscription/SubscriptionController.test.mjs b/services/web/test/unit/src/Subscription/SubscriptionController.test.mjs index fd2a77e2dc..3902d5c8d4 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionController.test.mjs +++ b/services/web/test/unit/src/Subscription/SubscriptionController.test.mjs @@ -685,7 +685,7 @@ describe('SubscriptionController', function () { ctx.next = sinon.stub() await expect( ctx.SubscriptionController.pauseSubscription(ctx.req, ctx.res, ctx.next) - ).to.be.rejectedWith('Invalid request parameters') + ).to.be.rejectedWith('Invalid params') }) it('should throw an error if an invalid pause length is provided', async function (ctx) { diff --git a/services/web/test/unit/src/Tags/TagsController.test.mjs b/services/web/test/unit/src/Tags/TagsController.test.mjs index b04457cd0f..9a8c249f9f 100644 --- a/services/web/test/unit/src/Tags/TagsController.test.mjs +++ b/services/web/test/unit/src/Tags/TagsController.test.mjs @@ -1,6 +1,6 @@ import { assert, beforeEach, describe, it, vi } from 'vitest' import sinon from 'sinon' -import { InvalidRequestError } from '@overleaf/validation-tools' +import { ZodError } from 'zod' const modulePath = '../../../../app/src/Features/Tags/TagsController.mjs' @@ -202,7 +202,7 @@ describe('TagsController', function () { it('without a name', function (ctx) { ctx.req.body = { name: undefined } ctx.TagsController.renameTag(ctx.req, ctx.res).should.be.rejectedWith( - InvalidRequestError + ZodError ) }) })