From f26a48fc4c6c6091ba6aa34d70a7d615ef34adef Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Fri, 14 Aug 2026 08:17:19 +0200 Subject: [PATCH] Merge pull request #36221 from overleaf/jpa-zod-web-b2c [web] b2c: adopt strict zod schema and extend test coverage GitOrigin-RevId: 5c612e3ce83b81cfff7b8f1c5bc4522d6feb665a --- .../src/Features/Referal/ReferalConnect.mjs | 103 +++-- .../Subscription/SubscriptionController.mjs | 148 ++++++- .../SubscriptionGroupController.mjs | 119 +++++- .../Templates/TemplatesController.mjs | 74 +++- .../Templates/TemplatesMiddleware.mjs | 27 +- .../Features/Templates/TemplatesRouter.mjs | 25 +- .../Features/Tutorial/TutorialController.mjs | 58 ++- .../app/src/Features/User/UserController.mjs | 125 +++++- .../Features/User/UserEmailsController.mjs | 111 ++++- .../src/Features/User/UserInfoController.mjs | 13 +- .../src/Features/User/UserPagesController.mjs | 32 +- .../acceptance/src/AddSecondaryEmailTests.mjs | 39 ++ .../web/test/acceptance/src/TutorialTests.mjs | 79 ++++ .../unit/src/Referal/ReferalConnect.test.mjs | 59 ++- .../SubscriptionController.test.mjs | 402 +++++++++++++++++- .../SubscriptionGroupController.test.mjs | 204 ++++++++- .../TeamInvitesController.test.mjs | 68 ++- .../Templates/TemplatesController.test.mjs | 24 +- .../src/Tutorial/TutorialController.test.mjs | 171 ++++++++ .../User/ThirdPartyIdentityManager.test.mjs | 40 ++ .../unit/src/User/UserController.test.mjs | 145 ++++++- .../src/User/UserEmailsController.test.mjs | 20 + .../unit/src/User/UserInfoController.test.mjs | 24 +- .../src/User/UserPagesController.test.mjs | 107 ++++- 24 files changed, 2038 insertions(+), 179 deletions(-) create mode 100644 services/web/test/acceptance/src/TutorialTests.mjs create mode 100644 services/web/test/unit/src/Tutorial/TutorialController.test.mjs diff --git a/services/web/app/src/Features/Referal/ReferalConnect.mjs b/services/web/app/src/Features/Referal/ReferalConnect.mjs index 7b65151653..2ff69d7353 100644 --- a/services/web/app/src/Features/Referal/ReferalConnect.mjs +++ b/services/web/app/src/Features/Referal/ReferalConnect.mjs @@ -1,49 +1,68 @@ +import { z, parseReq } from '../../infrastructure/Validation.mjs' + +// Mounted globally ahead of every route's own schema (see +// `webRouter.use(ReferalConnect.use)` in infrastructure/Server.mjs), so this +// stays a non-strict, best-effort read of a handful of short referral/ +// affiliate tracking query params -- rejecting/stripping fields that belong +// to the route's own schema isn't this middleware's job. `rm`/`rs` values +// outside the switch statements below are silently tolerated (left +// unmapped), so those stay bare optional strings rather than z.enum(...). +const referalQuerySchema = z.object({ + query: z.object({ + referal: z.string().optional(), + r: z.string().optional(), + fb_ref: z.string().optional(), + rm: z.string().optional(), + rs: z.string().optional(), + }), +}) + export default { use(req, res, next) { - if (req.query != null) { - if (req.query.referal != null) { - req.session.referal_id = req.query.referal - } else if (req.query.r != null) { - // Short hand for referal - req.session.referal_id = req.query.r - } else if (req.query.fb_ref != null) { - req.session.referal_id = req.query.fb_ref - } + const { query } = parseReq(req, referalQuerySchema, { logOnly: true }) - if (req.query.rm != null) { - // referal medium e.g. twitter, facebook, email - switch (req.query.rm) { - case 'fb': - req.session.referal_medium = 'facebook' - break - case 't': - req.session.referal_medium = 'twitter' - break - case 'gp': - req.session.referal_medium = 'google_plus' - break - case 'e': - req.session.referal_medium = 'email' - break - case 'd': - req.session.referal_medium = 'direct' - break - } - } + if (query.referal != null) { + req.session.referal_id = query.referal + } else if (query.r != null) { + // Short hand for referal + req.session.referal_id = query.r + } else if (query.fb_ref != null) { + req.session.referal_id = query.fb_ref + } - if (req.query.rs != null) { - // referal source e.g. project share, bonus - switch (req.query.rs) { - case 'b': - req.session.referal_source = 'bonus' - break - case 'ps': - req.session.referal_source = 'public_share' - break - case 'ci': - req.session.referal_source = 'collaborator_invite' - break - } + if (query.rm != null) { + // referal medium e.g. twitter, facebook, email + switch (query.rm) { + case 'fb': + req.session.referal_medium = 'facebook' + break + case 't': + req.session.referal_medium = 'twitter' + break + case 'gp': + req.session.referal_medium = 'google_plus' + break + case 'e': + req.session.referal_medium = 'email' + break + case 'd': + req.session.referal_medium = 'direct' + break + } + } + + if (query.rs != null) { + // referal source e.g. project share, bonus + switch (query.rs) { + case 'b': + req.session.referal_source = 'bonus' + break + case 'ps': + req.session.referal_source = 'public_share' + break + case 'ci': + req.session.referal_source = 'collaborator_invite' + break } } diff --git a/services/web/app/src/Features/Subscription/SubscriptionController.mjs b/services/web/app/src/Features/Subscription/SubscriptionController.mjs index e20084fed9..8c3631997c 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionController.mjs +++ b/services/web/app/src/Features/Subscription/SubscriptionController.mjs @@ -28,7 +28,7 @@ import { DEFAULT_PRICE_VERSION } from './PriceVersions.mjs' import { User } from '../../models/User.mjs' import UserGetter from '../User/UserGetter.mjs' import { sanitizeSessionUserForFrontEnd } from '../../infrastructure/FrontEndUser.mjs' -import { z, parseReq } from '../../infrastructure/Validation.mjs' +import { z, zz, parseReq } from '../../infrastructure/Validation.mjs' import { PaymentProviderSubscriptionChange } from './PaymentProviderEntities.mjs' const { AddOnNotPresentError, MultiplePendingChangesError } = Errors @@ -149,11 +149,24 @@ function formatGroupPlansDataForDash() { } } +const userSubscriptionPageSchema = z.object({ + query: z.object({ + // rendered verbatim into the subscription dashboard; not consumed as a + // real error-code enum by this handler. + errorCode: z.string().optional(), + // not consumed + hasSubscription: z.stringbool().optional(), + }), +}) + /** * @param {any} req * @param {any} res */ async function userSubscriptionPage(req, res) { + const { query } = parseReq(req, userSubscriptionPageSchema, { + logOnly: true, + }) const user = SessionManager.getSessionUser(req.session) await SplitTestHandler.promises.getAssignment(req, res, 'sharing-updates') await SplitTestHandler.promises.getAssignment( @@ -200,7 +213,7 @@ async function userSubscriptionPage(req, res) { const userCanExtendTrial = ( await Modules.promises.hooks.fire('userCanExtendTrial', user) )?.[0] - const redirectedPaymentErrorCode = req.query.errorCode + const redirectedPaymentErrorCode = query.errorCode const isInTrial = SubscriptionHelper.isInTrial( personalSubscription?.payment?.trialEndsAt ) @@ -338,11 +351,35 @@ async function userSubscriptionPage(req, res) { res.render('subscriptions/dashboard-react', data) } +const successfulSubscriptionSchema = z.object({ + query: z.object({ + // only ever sent as the literal 'true', or omitted entirely -- see + // callers in modules/subscriptions/.../root.tsx. + upgrade: z.stringbool().optional(), + }), +}) +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +// `query.upgrade` is compared against the literal boolean `true` below, so +// a raw passthrough still needs to produce a real boolean. +const successfulSubscriptionFallbackSchema = z.object({ + query: z.object({ + upgrade: z + .unknown() + .optional() + .transform(v => v === 'true' || v === true), + }), +}) + /** * @param {any} req * @param {any} res */ async function successfulSubscription(req, res) { + const { query } = parseReq(req, successfulSubscriptionSchema, { + logOnly: true, + fallbackSchema: successfulSubscriptionFallbackSchema, + }) const user = SessionManager.getSessionUser(req.session) if (!user) { throw new Error('User is not logged in') @@ -354,7 +391,7 @@ async function successfulSubscription(req, res) { ) const postCheckoutRedirect = req.session?.postCheckoutRedirect - const isUpgrade = req.query.upgrade === 'true' + const isUpgrade = query.upgrade === true if (!personalSubscription) { res.redirect('/user/subscription/plans') @@ -382,7 +419,7 @@ async function successfulSubscription(req, res) { } const pauseSubscriptionSchema = z.object({ - params: z.object({ + params: z.strictObject({ pauseCycles: z.coerce.number().int().max(12), }), }) @@ -520,12 +557,21 @@ function cancelV1Subscription(req, res, next) { ) } +const previewAddonPurchaseSchema = z.object({ + params: z.strictObject({ + addOnCode: z.string(), + }), +}) + /** * @param {any} req * @param {any} res */ async function previewAddonPurchase(req, res) { - const addOnCode = req.params.addOnCode + const { params } = parseReq(req, previewAddonPurchaseSchema, { + logOnly: true, + }) + const addOnCode = params.addOnCode if (addOnCode !== AI_ADD_ON_CODE) { return HttpErrorHandler.notFound(req, res, `Unknown add-on: ${addOnCode}`) @@ -546,7 +592,7 @@ async function purchaseAddon(req, res, next) { } const removeAddonSchema = z.object({ - params: z.object({ + params: z.strictObject({ addOnCode: z.string(), }), }) @@ -601,7 +647,7 @@ async function removeAddon(req, res, next) { } const reactivateAddonSchema = z.object({ - params: z.object({ + params: z.strictObject({ addOnCode: z.string(), }), }) @@ -639,13 +685,25 @@ async function reactivateAddon(req, res) { } } +const previewSubscriptionSchema = z.object({ + query: z.object({ + planCode: z.string().optional(), + // rendered verbatim into the preview page; not consumed as a real + // error-code enum by this handler. + errorCode: z.string().optional(), + }), +}) + /** * @param {any} req * @param {any} res * @param {any} next */ async function previewSubscription(req, res, next) { - const planCode = req.query.planCode + const { query } = parseReq(req, previewSubscriptionSchema, { + logOnly: true, + }) + const planCode = query.planCode if (!planCode) { return HttpErrorHandler.notFound(req, res, 'Missing plan code') } @@ -698,7 +756,7 @@ async function previewSubscription(req, res, next) { res.render('subscriptions/preview-change', { changePreview, - redirectedPaymentErrorCode: req.query.errorCode, + redirectedPaymentErrorCode: query.errorCode, trialDisabledReason, }) } @@ -777,15 +835,27 @@ function reactivateSubscription(req, res, next) { }) } +// Recurly's webhook body is `{ : { ...event-specific fields } }` +// -- the event name is one of an open-ended set defined by Recurly (this +// handler only actively branches on a known subset; anything else falls +// through to the generic 200 response below), and the payload shape varies +// per event type. This is a genuinely open map, not a shape we can name +// field-by-field. +const recurlyCallbackSchema = z.object({ + body: z.record(z.string(), z.unknown()), +}) + /** * @param {any} req * @param {any} res * @param {any} next */ function recurlyCallback(req, res, next) { - logger.debug({ data: req.body }, 'received recurly callback') - const event = Object.keys(req.body)[0] - const eventData = req.body[event] + const { body } = parseReq(req, recurlyCallbackSchema, { logOnly: true }) + logger.debug({ data: body }, 'received recurly callback') + const event = Object.keys(body)[0] + /** @type {any} the shape varies per Recurly event type -- see the schema comment above */ + const eventData = body[event] RecurlyEventHandler.sendRecurlyAnalyticsEvent(event, eventData).catch(error => logger.error( @@ -885,36 +955,63 @@ function recurlyNotificationParser(req, res, next) { ) } +const refreshUserFeaturesSchema = z.object({ + params: z.strictObject({ + user_id: zz.objectId(), + }), +}) + /** * @param {any} req * @param {any} res */ async function refreshUserFeatures(req, res) { - const { user_id: userId } = req.params + const { params } = parseReq(req, refreshUserFeaturesSchema, { + logOnly: true, + }) + const { user_id: userId } = params await FeaturesUpdater.promises.refreshFeatures(userId, 'acceptance-test') res.sendStatus(200) } +// This is invoked as a shared helper from several different routes' +// handlers (PlansController, InterstitialPaymentController, +// PaymentController), not mounted as a route itself -- like middleware, it +// validates only the fields it reads, non-strictly, so it doesn't reject +// fields that belong to whichever route's own schema actually owns the +// request. +const getRecommendedCurrencySchema = z.object({ + query: z.object({ + // only trusted for site admins (checked below); an override for + // testing/support purposes. + ip: z.ipv4().optional(), + currency: z.string().optional(), + }), +}) + /** * @param {any} req * @param {any} res * @returns {Promise<{currency: CurrencyCode, recommendedCurrency: CurrencyCode, countryCode: string|undefined}>} */ async function getRecommendedCurrency(req, res) { + const { query } = parseReq(req, getRecommendedCurrencySchema, { + logOnly: true, + }) const userId = SessionManager.getLoggedInUserId(req.session) let ip = req.ip if ( - req.query?.ip && + query?.ip && (await AuthorizationManager.promises.isUserSiteAdmin(userId)) ) { - ip = req.query.ip + ip = query.ip } const currencyLookup = await GeoIpLookup.promises.getCurrencyCode(ip) const countryCode = currencyLookup.countryCode const recommendedCurrency = currencyLookup.currencyCode let currency = null - const queryCurrency = req.query.currency?.toUpperCase() + const queryCurrency = query.currency?.toUpperCase() if (queryCurrency && GeoIpLookup.isValidCurrencyParam(queryCurrency)) { currency = queryCurrency } else if (recommendedCurrency) { @@ -922,24 +1019,37 @@ async function getRecommendedCurrency(req, res) { } return { - currency, + // `currency` can genuinely be null (no query override and no + // GeoIP-recommended currency); the return type below is looser than + // the cast. + currency: /** @type {any} */ (currency), recommendedCurrency, countryCode, } } +// Shared helper, same caveat as getRecommendedCurrency above. +const getLatamCountryBannerDetailsSchema = z.object({ + query: z.object({ + ip: z.ipv4().optional(), + }), +}) + /** * @param {any} req * @param {any} res */ async function getLatamCountryBannerDetails(req, res) { + const { query } = parseReq(req, getLatamCountryBannerDetailsSchema, { + logOnly: true, + }) const userId = SessionManager.getLoggedInUserId(req.session) let ip = req.ip if ( - req.query?.ip && + query?.ip && (await AuthorizationManager.promises.isUserSiteAdmin(userId)) ) { - ip = req.query.ip + ip = query.ip } const currencyLookup = await GeoIpLookup.promises.getCurrencyCode(ip) const countryCode = currencyLookup.countryCode diff --git a/services/web/app/src/Features/Subscription/SubscriptionGroupController.mjs b/services/web/app/src/Features/Subscription/SubscriptionGroupController.mjs index 36388c9de5..3a3625f5c3 100644 --- a/services/web/app/src/Features/Subscription/SubscriptionGroupController.mjs +++ b/services/web/app/src/Features/Subscription/SubscriptionGroupController.mjs @@ -10,7 +10,7 @@ import { expressify } from '@overleaf/promise-utils' import Modules from '../../infrastructure/Modules.mjs' import UserGetter from '../User/UserGetter.mjs' import { Subscription } from '../../models/Subscription.mjs' -import { z, parseReq } from '../../infrastructure/Validation.mjs' +import { z, zz, parseReq } from '../../infrastructure/Validation.mjs' import { isProfessionalGroupPlan } from './PlansHelper.mjs' import { MissingBillingInfoError, @@ -30,14 +30,28 @@ const MAX_NUMBER_OF_PO_NUMBER_CHARACTERS = 50 * @import { Subscription } from "../../../../types/subscription/dashboard/subscription.js" */ +const removeUserFromGroupSchema = z.object({ + // mounted at /manage/groups/:id/user/:user_id -- `id` is consumed + // upstream by UserMembershipMiddleware.requireGroupMemberManagement() + // (which sets req.entity), but it's still present on req.params, so the + // strict schema below has to name it too. + params: z.strictObject({ + id: zz.objectId(), + user_id: zz.objectId(), + }), +}) + /** * @param {import("express").Request} req * @param {import("express").Response} res * @returns {Promise} */ async function removeUserFromGroup(req, res) { + const { params } = parseReq(req, removeUserFromGroupSchema, { + logOnly: true, + }) const subscription = req.entity - const userToRemoveId = req.params.user_id + const userToRemoveId = params.user_id const loggedInUserId = SessionManager.getLoggedInUserId(req.session) const subscriptionId = subscription._id logger.debug( @@ -52,15 +66,24 @@ async function removeUserFromGroup(req, res) { }) } +const removeSelfFromGroupSchema = z.object({ + query: z.object({ + subscriptionId: zz.objectId(), + }), +}) + /** * @param {import("express").Request} req * @param {import("express").Response} res * @returns {Promise} */ async function removeSelfFromGroup(req, res) { + const { query } = parseReq(req, removeSelfFromGroupSchema, { + logOnly: true, + }) const userToRemoveId = SessionManager.getLoggedInUserId(req.session) const subscription = await SubscriptionLocator.promises.getSubscription( - req.query.subscriptionId + query.subscriptionId ) await _removeUserFromGroup(req, res, { @@ -137,12 +160,23 @@ async function _removeUserFromGroup( res.sendStatus(200) } +const addSeatsToGroupSubscriptionSchema = z.object({ + query: z.object({ + // rendered verbatim into the add-seats page; not consumed as a real + // error-code enum by this handler. + errorCode: z.string().optional(), + }), +}) + /** * @param {import("express").Request} req * @param {import("express").Response} res * @returns {Promise} */ async function addSeatsToGroupSubscription(req, res) { + const { query } = parseReq(req, addSeatsToGroupSubscriptionSchema, { + logOnly: true, + }) try { const userId = SessionManager.getLoggedInUserId(req.session) const { subscription, paymentProviderSubscription, plan } = @@ -171,7 +205,7 @@ async function addSeatsToGroupSubscription(req, res) { isProfessional: isProfessionalGroupPlan(subscription), isCollectionMethodManual: paymentProviderSubscription.isCollectionMethodManual, - redirectedPaymentErrorCode: req.query.errorCode, + redirectedPaymentErrorCode: query.errorCode, }) } catch (error) { if (error instanceof MissingBillingInfoError) { @@ -202,7 +236,17 @@ async function addSeatsToGroupSubscription(req, res) { } } +const addSeatsBodySchema = z.strictObject({ + adding: z.number().int().min(1).max(MAX_NUMBER_OF_USERS), + poNumber: z.string().max(MAX_NUMBER_OF_PO_NUMBER_CHARACTERS).optional(), +}) + const previewAddSeatsSubscriptionChangeSchema = z.object({ + body: addSeatsBodySchema, +}) +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const previewAddSeatsSubscriptionChangeFallbackSchema = z.object({ body: z.object({ adding: z.number().int().min(1).max(MAX_NUMBER_OF_USERS), poNumber: z.string().max(MAX_NUMBER_OF_PO_NUMBER_CHARACTERS).optional(), @@ -215,7 +259,9 @@ const previewAddSeatsSubscriptionChangeSchema = z.object({ * @returns {Promise} */ async function previewAddSeatsSubscriptionChange(req, res) { - const { body } = parseReq(req, previewAddSeatsSubscriptionChangeSchema) + const { body } = parseReq(req, previewAddSeatsSubscriptionChangeSchema, { + fallbackSchema: previewAddSeatsSubscriptionChangeFallbackSchema, + }) try { const userId = SessionManager.getLoggedInUserId(req.session) const preview = @@ -251,19 +297,26 @@ async function previewAddSeatsSubscriptionChange(req, res) { } } +const createAddSeatsSubscriptionChangeSchema = z.object({ + body: addSeatsBodySchema, +}) + /** * @param {import("express").Request} req * @param {import("express").Response} res * @returns {Promise} */ async function createAddSeatsSubscriptionChange(req, res) { + const { body } = parseReq(req, createAddSeatsSubscriptionChangeSchema, { + logOnly: true, + }) try { const userId = SessionManager.getLoggedInUserId(req.session) const create = await SubscriptionGroupHandler.promises.createAddSeatsSubscriptionChange( userId, - req.body.adding, - req.body.poNumber + body.adding, + body.poNumber ) res.json(create) @@ -281,7 +334,7 @@ async function createAddSeatsSubscriptionChange(req, res) { if (error instanceof SubtotalLimitExceededError) { return res.status(422).json({ code: 'subtotal_limit_exceeded', - adding: req.body.adding, + adding: body.adding, }) } @@ -303,6 +356,14 @@ async function createAddSeatsSubscriptionChange(req, res) { } const submitFormSchema = z.object({ + body: z.strictObject({ + adding: z.coerce.number().int().min(MAX_NUMBER_OF_USERS), + poNumber: z.string().optional(), + }), +}) +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const submitFormFallbackSchema = z.object({ body: z.object({ adding: z.coerce.number().int().min(MAX_NUMBER_OF_USERS), poNumber: z.string().optional(), @@ -310,7 +371,9 @@ const submitFormSchema = z.object({ }) async function submitForm(req, res) { - const { body } = parseReq(req, submitFormSchema) + const { body } = parseReq(req, submitFormSchema, { + fallbackSchema: submitFormFallbackSchema, + }) const { adding, poNumber } = body const userId = SessionManager.getLoggedInUserId(req.session) @@ -361,7 +424,18 @@ async function submitForm(req, res) { res.sendStatus(204) } +const subscriptionUpgradePageSchema = z.object({ + query: z.object({ + // rendered verbatim into the upgrade page; not consumed as a real + // error-code enum by this handler. + errorCode: z.string().optional(), + }), +}) + async function subscriptionUpgradePage(req, res) { + const { query } = parseReq(req, subscriptionUpgradePageSchema, { + logOnly: true, + }) try { const userId = SessionManager.getLoggedInUserId(req.session) const changePreview = @@ -373,7 +447,7 @@ async function subscriptionUpgradePage(req, res) { changePreview, totalLicenses: olSubscription.membersLimit, groupName: olSubscription.teamName, - redirectedPaymentErrorCode: req.query.errorCode, + redirectedPaymentErrorCode: query.errorCode, }) } catch (error) { if (error instanceof MissingBillingInfoError) { @@ -445,7 +519,17 @@ async function missingBillingInformation(req, res) { } } +const manuallyCollectedSubscriptionSchema = z.object({ + query: z.object({ + // rendered verbatim into the manually-collected-subscription page. + error_type: z.string().optional(), + }), +}) + async function manuallyCollectedSubscription(req, res) { + const { query } = parseReq(req, manuallyCollectedSubscriptionSchema, { + logOnly: true, + }) try { const userId = SessionManager.getLoggedInUserId(req.session) const subscription = @@ -453,7 +537,7 @@ async function manuallyCollectedSubscription(req, res) { res.render('subscriptions/manually-collected-subscription', { groupName: subscription.teamName, - errorType: req.query.error_type, + errorType: query.error_type, }) } catch (error) { logger.err( @@ -479,13 +563,24 @@ async function subtotalLimitExceeded(req, res) { } } +const getGroupPlanPerUserPricesSchema = z.object({ + query: z.object({ + // forwarded to the Stripe client as a currency filter; Stripe itself + // rejects an unrecognised code, so no enum is duplicated here. + currency: z.string().optional(), + }), +}) + async function getGroupPlanPerUserPrices(req, res) { + const { query } = parseReq(req, getGroupPlanPerUserPricesSchema, { + logOnly: true, + }) try { const userId = SessionManager.getLoggedInUserId(req.session) const prices = await Modules.promises.hooks.fire( 'getGroupPlanPerUserPrices', userId, - req.query.currency + query.currency ) return res.json(prices[0]) } catch (error) { diff --git a/services/web/app/src/Features/Templates/TemplatesController.mjs b/services/web/app/src/Features/Templates/TemplatesController.mjs index 63d10bdc5e..ae39720a50 100644 --- a/services/web/app/src/Features/Templates/TemplatesController.mjs +++ b/services/web/app/src/Features/Templates/TemplatesController.mjs @@ -9,31 +9,63 @@ import { parseReq, z } from '../../infrastructure/Validation.mjs' const numericId = z.string().regex(/^[0-9]+$/) const getV1TemplateSchema = z.object({ - params: z.object({ Template_version_id: numericId }), - query: z.object({ id: numericId }), + params: z.strictObject({ Template_version_id: numericId }), + query: z.object({ + id: numericId, + templateName: z.string().optional(), + latexEngine: z.string().optional(), + texImage: z.string().optional(), + mainFile: z.string().optional(), + brandVariationId: z.coerce.number().int().positive().optional(), + }), +}) + +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const getV1TemplateFallbackSchema = z.object({ + params: z.object({ Template_version_id: numericId }).passthrough(), + query: z.object({ id: numericId }).passthrough(), }) const createProjectFromV1TemplateSchema = z.object({ - body: z.object({ + body: z.strictObject({ templateId: numericId, templateVersionId: numericId, + brandVariationId: z.coerce.number().int().positive().optional(), + compiler: z.string().optional(), + mainFile: z.string().optional(), + templateName: z.string().optional(), + imageName: z.string().optional(), }), }) +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const createProjectFromV1TemplateFallbackSchema = z.object({ + body: z + .object({ + templateId: numericId, + templateVersionId: numericId, + }) + .passthrough(), +}) + const TemplatesController = { async getV1Template(req, res) { const { params: { Template_version_id: templateVersionId }, - query: { id: templateId }, - } = parseReq(req, getV1TemplateSchema) + query, + } = parseReq(req, getV1TemplateSchema, { + fallbackSchema: getV1TemplateFallbackSchema, + }) const data = { templateVersionId, - templateId, - name: req.query.templateName, - compiler: ProjectHelper.compilerFromV1Engine(req.query.latexEngine), - imageName: req.query.texImage, - mainFile: req.query.mainFile, - brandVariationId: req.query.brandVariationId, + templateId: query.id, + name: query.templateName, + compiler: ProjectHelper.compilerFromV1Engine(query.latexEngine), + imageName: query.texImage, + mainFile: query.mainFile, + brandVariationId: query.brandVariationId, } res.render( path.resolve( @@ -45,19 +77,19 @@ const TemplatesController = { }, async createProjectFromV1Template(req, res) { - const { - body: { templateId, templateVersionId }, - } = parseReq(req, createProjectFromV1TemplateSchema) + const { body } = parseReq(req, createProjectFromV1TemplateSchema, { + fallbackSchema: createProjectFromV1TemplateFallbackSchema, + }) const userId = SessionManager.getLoggedInUserId(req.session) const project = await TemplatesManager.promises.createProjectFromV1Template( - req.body.brandVariationId, - req.body.compiler, - req.body.mainFile, - templateId, - req.body.templateName, - templateVersionId, + body.brandVariationId, + body.compiler, + body.mainFile, + body.templateId, + body.templateName, + body.templateVersionId, userId, - req.body.imageName + body.imageName ) delete req.session.templateData if (!project) { diff --git a/services/web/app/src/Features/Templates/TemplatesMiddleware.mjs b/services/web/app/src/Features/Templates/TemplatesMiddleware.mjs index efa8bcb6ac..1f584b86d2 100644 --- a/services/web/app/src/Features/Templates/TemplatesMiddleware.mjs +++ b/services/web/app/src/Features/Templates/TemplatesMiddleware.mjs @@ -1,21 +1,20 @@ -/* eslint-disable - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ -import settings from '@overleaf/settings' +import { z, parseReq } from '../../infrastructure/Validation.mjs' -import logger from '@overleaf/logger' +const saveTemplateDataInSessionSchema = z.object({ + query: z.object({ + templateName: z.string().optional(), + }), +}) export default { saveTemplateDataInSession(req, res, next) { - if (req.query.templateName) { - req.session.templateData = req.query + const { query } = parseReq(req, saveTemplateDataInSessionSchema, { + logOnly: true, + }) + if (query.templateName) { + // only `templateName` is ever read back out of session.templateData + // (see UserPagesController.registerPage) — store just that field + req.session.templateData = { templateName: query.templateName } } return next() }, diff --git a/services/web/app/src/Features/Templates/TemplatesRouter.mjs b/services/web/app/src/Features/Templates/TemplatesRouter.mjs index 4579b04412..87a5c01ec3 100644 --- a/services/web/app/src/Features/Templates/TemplatesRouter.mjs +++ b/services/web/app/src/Features/Templates/TemplatesRouter.mjs @@ -4,22 +4,39 @@ import TemplatesMiddleware from './TemplatesMiddleware.mjs' import { RateLimiter } from '../../infrastructure/RateLimiter.mjs' import RateLimiterMiddleware from '../Security/RateLimiterMiddleware.mjs' import AnalyticsRegistrationSourceMiddleware from '../Analytics/AnalyticsRegistrationSourceMiddleware.mjs' +import { z, parseReq } from '../../infrastructure/Validation.mjs' const rateLimiter = new RateLimiter('create-project-from-template', { points: 20, duration: 60, }) +// Non-strict, and deliberately looser than TemplatesController.getV1Template's +// own numeric-id schema: this only threads the param through to +// AnalyticsRegistrationSourceMiddleware's session bookkeeping, and rejecting +// or reformatting it here isn't this middleware's job -- getV1Template +// (later in the same chain) still enforces the real numeric v1 +// template-version-id format for the route. +const templateVersionIdSchema = z.object({ + params: z.object({ + Template_version_id: z.string(), + }), +}) + export default { rateLimiter, apply(app) { app.get( '/project/new/template/:Template_version_id', - (req, res, next) => - AnalyticsRegistrationSourceMiddleware.setSource( + (req, res, next) => { + const { params } = parseReq(req, templateVersionIdSchema, { + logOnly: true, + }) + return AnalyticsRegistrationSourceMiddleware.setSource( 'template', - req.params.Template_version_id - )(req, res, next), + params.Template_version_id + )(req, res, next) + }, TemplatesMiddleware.saveTemplateDataInSession, AuthenticationController.requireLogin(), TemplatesController.getV1Template, diff --git a/services/web/app/src/Features/Tutorial/TutorialController.mjs b/services/web/app/src/Features/Tutorial/TutorialController.mjs index 0e0e15dbff..45673745c3 100644 --- a/services/web/app/src/Features/Tutorial/TutorialController.mjs +++ b/services/web/app/src/Features/Tutorial/TutorialController.mjs @@ -1,6 +1,7 @@ import SessionManager from '../Authentication/SessionManager.mjs' import TutorialHandler from './TutorialHandler.mjs' import { expressify } from '@overleaf/promise-utils' +import { z, zz, parseReq } from '../../infrastructure/Validation.mjs' const VALID_KEYS = [ 'react-history-buttons-tutorial', @@ -34,9 +35,49 @@ const VALID_KEYS = [ 'dimensions-rail-popover', ] +// tutorialKey is validated against the same known-key enum the handlers +// also check by hand below. Both the schema and the manual +// VALID_KEYS.includes() guard enforce the same allow-list during this +// rollout's logOnly phase, so an unrecognized key still 404s even while the +// schema failure alone would not throw. +const tutorialKeyParamsSchema = z.strictObject({ + tutorialKey: z.enum(VALID_KEYS), +}) + +const completeTutorialSchema = z.object({ + params: tutorialKeyParamsSchema, +}) + +const postponeTutorialSchema = z.object({ + params: tutorialKeyParamsSchema, + body: z.strictObject({ + postponedUntil: zz.datetime().optional(), + }), +}) + +// Rollout-temporary fallback (loosened primary schema; no zod validation +// existed for this route on main); delete when this route's +// REQ_VALIDATION_MODE instrumentation is removed. +const postponeTutorialFallbackSchema = z.object({ + params: z.object({ + tutorialKey: z.enum(VALID_KEYS), + }), + body: z.object({ + postponedUntil: z + .union([z.date(), z.string()]) + .optional() + .transform(dt => + dt === undefined ? undefined : dt instanceof Date ? dt : new Date(dt) + ), + }), +}) + async function completeTutorial(req, res, next) { const userId = SessionManager.getLoggedInUserId(req.session) - const tutorialKey = req.params.tutorialKey + const { params } = parseReq(req, completeTutorialSchema, { + logOnly: true, + }) + const { tutorialKey } = params if (!VALID_KEYS.includes(tutorialKey)) { return res.sendStatus(404) @@ -48,16 +89,21 @@ async function completeTutorial(req, res, next) { async function postponeTutorial(req, res, next) { const userId = SessionManager.getLoggedInUserId(req.session) - const tutorialKey = req.params.tutorialKey - let postponedUntil - if (req.body.postponedUntil) { - postponedUntil = new Date(req.body.postponedUntil) - } + const { params, body } = parseReq(req, postponeTutorialSchema, { + logOnly: true, + fallbackSchema: postponeTutorialFallbackSchema, + }) + const { tutorialKey } = params if (!VALID_KEYS.includes(tutorialKey)) { return res.sendStatus(404) } + let postponedUntil + if (body.postponedUntil) { + postponedUntil = new Date(body.postponedUntil) + } + await TutorialHandler.setTutorialState( userId, tutorialKey, diff --git a/services/web/app/src/Features/User/UserController.mjs b/services/web/app/src/Features/User/UserController.mjs index d2f7543709..19e1dcd960 100644 --- a/services/web/app/src/Features/User/UserController.mjs +++ b/services/web/app/src/Features/User/UserController.mjs @@ -7,7 +7,7 @@ import metrics from '@overleaf/metrics' import AuthenticationManager from '../Authentication/AuthenticationManager.mjs' import SessionManager from '../Authentication/SessionManager.mjs' import Features from '../../infrastructure/Features.mjs' -import { z, parseReq } from '../../infrastructure/Validation.mjs' +import { z, zz, parseReq } from '../../infrastructure/Validation.mjs' import UserAuditLogHandler from './UserAuditLogHandler.mjs' import UserSessionsManager from './UserSessionsManager.mjs' import UserUpdater from './UserUpdater.mjs' @@ -65,13 +65,22 @@ async function _ensureAffiliation(userId, emailData) { } } +const changePasswordSchema = z.object({ + body: z.strictObject({ + currentPassword: z.string().optional(), + newPassword1: z.string().optional(), + newPassword2: z.string().optional(), + }), +}) + async function changePassword(req, res, next) { metrics.inc('user.password-change') + const { body } = parseReq(req, changePasswordSchema, { logOnly: true }) const userId = SessionManager.getLoggedInUserId(req.session) const { user } = await AuthenticationManager.promises.authenticate( { _id: userId }, - req.body.currentPassword, + body.currentPassword, null, { enforceHIBPCheck: false } ) @@ -83,7 +92,7 @@ async function changePassword(req, res, next) { ) } - if (req.body.newPassword1 !== req.body.newPassword2) { + if (body.newPassword1 !== body.newPassword2) { return HttpErrorHandler.badRequest( req, res, @@ -94,7 +103,7 @@ async function changePassword(req, res, next) { try { await AuthenticationManager.promises.setUserPassword( user, - req.body.newPassword1 + body.newPassword1 ) } catch (error) { if (error.name === 'InvalidPasswordError') { @@ -192,9 +201,20 @@ async function ensureAffiliation(user) { await _ensureAffiliation(user._id, flaggedEmails[0]) } +const ensureAffiliationMiddlewareSchema = z.object({ + query: z.object({ + // just a truthiness flag on whether to run the affiliation check; the + // real caller sends the query string `?ensureAffiliation=true` + ensureAffiliation: z.coerce.boolean().optional(), + }), +}) + async function ensureAffiliationMiddleware(req, res, next) { let user - if (!Features.hasFeature('affiliations') || !req.query.ensureAffiliation) { + const { query } = parseReq(req, ensureAffiliationMiddlewareSchema, { + logOnly: true, + }) + if (!Features.hasFeature('affiliations') || !query.ensureAffiliation) { return next() } const userId = SessionManager.getLoggedInUserId(req.session) @@ -215,9 +235,17 @@ async function ensureAffiliationMiddleware(req, res, next) { return next() } +const tryDeleteUserSchema = z.object({ + body: z.strictObject({ + password: z.string().optional(), + }), +}) + async function tryDeleteUser(req, res, next) { const userId = SessionManager.getLoggedInUserId(req.session) - const { password } = req.body + const { password } = parseReq(req, tryDeleteUserSchema, { + logOnly: true, + }).body req.logger.addFields({ userId }) logger.debug({ userId }, 'trying to delete user account') @@ -334,6 +362,16 @@ async function unsubscribe(req, res, next) { } const refProviderSettingsSchema = z + .strictObject({ + enabled: z.boolean().optional(), + groups: z.array(z.strictObject({ id: z.string() })).optional(), + disablePersonalLibrary: z.boolean().optional(), + }) + .optional() + +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const refProviderSettingsFallbackSchema = z .object({ enabled: z.boolean().optional(), groups: z.array(z.object({ id: z.string() })).optional(), @@ -342,20 +380,62 @@ const refProviderSettingsSchema = z .optional() const updateUserSettingsSchema = z.object({ + // the frontend posts one changed setting at a time (`{[key]: value}`), so + // every field here is optional + body: z.strictObject({ + first_name: z.string().max(255).nullish(), + last_name: z.string().max(255).nullish(), + role: z.string().optional(), + institution: z.string().optional(), + email: z.string().optional(), + mode: z.string().optional(), + editorTheme: z.string().optional(), + editorLightTheme: z.string().optional(), + editorDarkTheme: z.string().optional(), + overallTheme: z.string().optional(), + fontSize: z.number().optional(), + autoComplete: z.boolean().optional(), + autoPairDelimiters: z.boolean().optional(), + spellCheckLanguage: z.string().optional(), + pdfViewer: z.string().optional(), + syntaxValidation: z.boolean().optional(), + // these six settings are cast with `Boolean(...)` below rather than + // checked for a strict boolean, so any truthy/falsy value is accepted + // here too + previewTabs: z.coerce.boolean().optional(), + fontFamily: z.string().optional(), + lineHeight: z.string().optional(), + mathPreview: z.boolean().optional(), + breadcrumbs: z.coerce.boolean().optional(), + editorTabs: z.coerce.boolean().optional(), + nonBlinkingCursor: z.coerce.boolean().optional(), + referencesSearchMode: z.string().optional(), + darkModePdf: z.coerce.boolean().optional(), + floatingMenu: z.coerce.boolean().optional(), + zotero: refProviderSettingsSchema, + mendeley: refProviderSettingsSchema, + papers: refProviderSettingsSchema, + }), +}) + +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const updateUserSettingsFallbackSchema = z.object({ body: z .object({ first_name: z.string().max(255).nullish(), last_name: z.string().max(255).nullish(), - zotero: refProviderSettingsSchema, - mendeley: refProviderSettingsSchema, - papers: refProviderSettingsSchema, + zotero: refProviderSettingsFallbackSchema, + mendeley: refProviderSettingsFallbackSchema, + papers: refProviderSettingsFallbackSchema, }) .passthrough(), - // TODO: complete the schema and remove the passthrough }) async function updateUserSettings(req, res, next) { - const { body } = parseReq(req, updateUserSettingsSchema) + const { body } = parseReq(req, updateUserSettingsSchema, { + fallbackSchema: updateUserSettingsFallbackSchema, + }) const userId = SessionManager.getLoggedInUserId(req.session) req.logger.addFields({ userId }) @@ -541,9 +621,16 @@ async function doLogout(req) { } } +const logoutSchema = z.object({ + body: z.strictObject({ + redirect: z.string().optional(), + }), +}) + async function logout(req, res, next) { - const requestedRedirect = req.body.redirect - ? UrlHelper.getSafeRedirectPath(req.body.redirect) + const { body } = parseReq(req, logoutSchema, { logOnly: true }) + const requestedRedirect = body.redirect + ? UrlHelper.getSafeRedirectPath(body.redirect) : undefined const redirectUrl = requestedRedirect || '/login' @@ -556,9 +643,17 @@ async function logout(req, res, next) { } } +const expireDeletedUserSchema = z.object({ + params: z.strictObject({ + userId: zz.objectId(), + }), +}) + async function expireDeletedUser(req, res, next) { - const userId = req.params.userId - await UserDeleter.promises.expireDeletedUser(userId) + const { params } = parseReq(req, expireDeletedUserSchema, { + logOnly: true, + }) + await UserDeleter.promises.expireDeletedUser(params.userId) res.sendStatus(204) } diff --git a/services/web/app/src/Features/User/UserEmailsController.mjs b/services/web/app/src/Features/User/UserEmailsController.mjs index 6ece5b63fe..794bf2180a 100644 --- a/services/web/app/src/Features/User/UserEmailsController.mjs +++ b/services/web/app/src/Features/User/UserEmailsController.mjs @@ -20,6 +20,7 @@ import { RateLimiter } from '../../infrastructure/RateLimiter.mjs' import Features from '../../infrastructure/Features.mjs' import tsscmp from 'tsscmp' import Modules from '../../infrastructure/Modules.mjs' +import { z, parseReq } from '../../infrastructure/Validation.mjs' const AUDIT_LOG_TOKEN_PREFIX_LENGTH = 10 @@ -54,9 +55,18 @@ async function _sendSecurityAlertEmail(user, email) { await EmailHandler.promises.sendEmail('securityAlert', emailOptions) } +const sendExistingEmailConfirmationCodeSchema = z.object({ + body: z.strictObject({ + email: z.string().optional(), + }), +}) + async function sendExistingEmailConfirmationCode(req, res) { const userId = SessionManager.getLoggedInUserId(req.session) - const email = EmailHelper.parseEmail(req.body.email) + const { body } = parseReq(req, sendExistingEmailConfirmationCodeSchema, { + logOnly: true, + }) + const email = EmailHelper.parseEmail(body.email) if (!email) { return res.sendStatus(400) } @@ -71,6 +81,23 @@ async function sendExistingEmailConfirmationCode(req, res) { res.sendStatus(204) } +const universitySchema = z + .strictObject({ + id: z.number().optional(), + name: z.string().optional(), + country_code: z.string().optional(), + }) + .optional() + +const addWithConfirmationCodeSchema = z.object({ + body: z.strictObject({ + email: z.string().optional(), + university: universitySchema, + role: z.string().optional(), + department: z.string().optional(), + }), +}) + /** * This method is for adding a secondary email to be confirmed via a code. */ @@ -78,11 +105,14 @@ async function addWithConfirmationCode(req, res) { delete req.session.pendingSecondaryEmail const userId = SessionManager.getLoggedInUserId(req.session) - const email = EmailHelper.parseEmail(req.body.email) + const { body } = parseReq(req, addWithConfirmationCodeSchema, { + logOnly: true, + }) + const email = EmailHelper.parseEmail(body.email) const affiliationOptions = { - university: req.body.university, - role: req.body.role, - department: req.body.department, + university: body.university, + role: body.role, + department: body.department, } if (!email) { @@ -175,6 +205,12 @@ async function sendCodeAndStoreInSession( } } +const checkConfirmationCodeSchema = z.object({ + body: z.strictObject({ + code: z.string().optional(), + }), +}) + /** * @param {string} sessionKey * @param {(req: import('express').Request, user: any, email: string, affiliationOptions: any) => Promise} beforeConfirmEmail @@ -183,7 +219,9 @@ async function sendCodeAndStoreInSession( const _checkConfirmationCode = (sessionKey, beforeConfirmEmail) => async (req, res) => { const userId = SessionManager.getLoggedInUserId(req.session) - const code = req.body.code + const { code } = parseReq(req, checkConfirmationCodeSchema, { + logOnly: true, + }).body const user = await UserGetter.promises.getUser(userId, { email: 1, 'emails.email': 1, @@ -481,16 +519,30 @@ async function primaryEmailCheck(req, res) { AsyncFormHelper.redirect(req, res, '/project') } +const showConfirmSchema = z.object({ + query: z.object({ + token: z.string().optional(), + }), +}) + async function showConfirm(req, res, next) { + const { query } = parseReq(req, showConfirmSchema, { logOnly: true }) res.render('user/confirm_email', { - token: req.query.token, + token: query.token, title: 'confirm_email', }) } +const removeEmailSchema = z.object({ + body: z.strictObject({ + email: z.string().optional(), + }), +}) + async function remove(req, res) { const userId = SessionManager.getLoggedInUserId(req.session) - const email = EmailHelper.parseEmail(req.body.email) + const { body } = parseReq(req, removeEmailSchema, { logOnly: true }) + const email = EmailHelper.parseEmail(body.email) if (!email) { return res.sendStatus(422) } @@ -502,9 +554,21 @@ async function remove(req, res) { res.sendStatus(200) } +const setDefaultEmailSchema = z.object({ + body: z.strictObject({ + email: z.string().optional(), + }), + query: z.object({ + 'delete-unconfirmed-primary': z.string().optional(), + }), +}) + async function setDefault(req, res, next) { const userId = SessionManager.getLoggedInUserId(req.session) - const email = EmailHelper.parseEmail(req.body.email) + const { body, query } = parseReq(req, setDefaultEmailSchema, { + logOnly: true, + }) + const email = EmailHelper.parseEmail(body.email) if (!email) { return res.sendStatus(422) @@ -516,7 +580,7 @@ async function setDefault(req, res, next) { ) const primaryEmailData = emails?.find(email => email.email === oldDefault) const deleteOldEmail = - req.query['delete-unconfirmed-primary'] !== undefined && + query['delete-unconfirmed-primary'] !== undefined && primaryEmailData && !primaryEmailData.confirmedAt @@ -550,7 +614,7 @@ async function setDefault(req, res, next) { ) } if ( - req.query['delete-unconfirmed-primary'] !== undefined && + query['delete-unconfirmed-primary'] !== undefined && primaryEmailData && !primaryEmailData.confirmedAt ) { @@ -569,6 +633,20 @@ async function setDefault(req, res, next) { res.sendStatus(200) } +const endorseEmailSchema = z.object({ + body: z.strictObject({ + email: z.string().optional(), + role: z.string().optional(), + department: z.string().optional(), + }), +}) + +const confirmEmailSchema = z.object({ + body: z.strictObject({ + token: z.string().nullish(), + }), +}) + const UserEmailsController = { list(req, res, next) { const userId = SessionManager.getLoggedInUserId(req.session) @@ -604,7 +682,8 @@ const UserEmailsController = { endorse(req, res, next) { const userId = SessionManager.getLoggedInUserId(req.session) - const email = EmailHelper.parseEmail(req.body.email) + const { body } = parseReq(req, endorseEmailSchema, { logOnly: true }) + const email = EmailHelper.parseEmail(body.email) if (!email) { return res.sendStatus(422) } @@ -612,8 +691,8 @@ const UserEmailsController = { InstitutionsAPI.endorseAffiliation( userId, email, - req.body.role, - req.body.department, + body.role, + body.department, function (error) { if (error) { return next(error) @@ -638,7 +717,9 @@ const UserEmailsController = { showConfirm: expressify(showConfirm), confirm(req, res, next) { - const { token } = req.body + const { token } = parseReq(req, confirmEmailSchema, { + logOnly: true, + }).body if (!token) { return res.status(422).json({ message: req.i18n.translate('confirmation_link_broken'), diff --git a/services/web/app/src/Features/User/UserInfoController.mjs b/services/web/app/src/Features/User/UserInfoController.mjs index 1e2b7e7d65..9992a9d65f 100644 --- a/services/web/app/src/Features/User/UserInfoController.mjs +++ b/services/web/app/src/Features/User/UserInfoController.mjs @@ -2,9 +2,18 @@ import UserGetter from './UserGetter.mjs' import SessionManager from '../Authentication/SessionManager.mjs' import mongodb from 'mongodb-legacy' import { expressify } from '@overleaf/promise-utils' +import { z, zz, parseReq } from '../../infrastructure/Validation.mjs' const { ObjectId } = mongodb +// user_id is either a real Mongo ObjectId or a legacy v1-only numeric +// (overleaf.id) id. +const getPersonalInfoSchema = z.object({ + params: z.strictObject({ + user_id: zz.objectId().or(z.coerce.number()), + }), +}) + function getLoggedInUsersPersonalInfo(req, res, next) { const userId = SessionManager.getLoggedInUserId(req.session) if (!userId) { @@ -30,8 +39,10 @@ function getLoggedInUsersPersonalInfo(req, res, next) { } function getPersonalInfo(req, res, next) { + const { params } = parseReq(req, getPersonalInfoSchema, { logOnly: true }) + let query - const userId = req.params.user_id + const userId = params.user_id if (/^\d+$/.test(userId)) { query = { 'overleaf.id': parseInt(userId, 10) } diff --git a/services/web/app/src/Features/User/UserPagesController.mjs b/services/web/app/src/Features/User/UserPagesController.mjs index 8fe4601bc3..e3049aba2d 100644 --- a/services/web/app/src/Features/User/UserPagesController.mjs +++ b/services/web/app/src/Features/User/UserPagesController.mjs @@ -11,6 +11,11 @@ import { expressify } from '@overleaf/promise-utils' import Features from '../../infrastructure/Features.mjs' import Modules from '../../infrastructure/Modules.mjs' import SplitTestHandler from '../SplitTests/SplitTestHandler.mjs' +import { + z, + parseReq, + getRawReqInput, +} from '../../infrastructure/Validation.mjs' function popSessionValue(session, key) { const value = session[key] @@ -20,9 +25,17 @@ function popSessionValue(session, key) { return value } +const settingsPageSchema = z.object({ + query: z.object({ + remove: z.string().optional(), + 'oauth-complete': z.string().optional(), + }), +}) + async function settingsPage(req, res) { + const { query } = parseReq(req, settingsPageSchema, { logOnly: true }) const userId = SessionManager.getLoggedInUserId(req.session) - const reconfirmationRemoveEmail = req.query.remove + const reconfirmationRemoveEmail = query.remove // SSO delete req.session.ssoError const ssoErrorMessage = popSessionValue(req.session, 'ssoErrorMessage') @@ -247,6 +260,12 @@ async function emailPreferencesPage(req, res) { }) } +const loginPageSchema = z.object({ + query: z.object({ + redir: z.string().optional(), + }), +}) + const UserPagesController = { accountSuspended: expressify(accountSuspended), logout: expressify(logout), @@ -268,16 +287,21 @@ const UserPagesController = { }, loginPage(req, res) { + const { query } = parseReq(req, loginPageSchema, { logOnly: true }) + // if user is being sent to /login with explicit redirect (redir=/foo), // such as being sent from the editor to /login, then set the redirect explicitly if ( - req.query.redir != null && + query.redir != null && AuthenticationController.getRedirectFromSession(req) == null ) { - AuthenticationController.setRedirectInSession(req, req.query.redir) + AuthenticationController.setRedirectInSession(req, query.redir) } const metadata = { robotsNoindexNofollow: false } - if (Object.keys(req.query).length !== 0) { + // any query param at all (not just `redir`) marks this page as noindex + // -- e.g. error/SSO-flow flags added by other callers -- read the raw + // query here rather than by name (case 1: verbatim forwarding) + if (Object.keys(getRawReqInput(req).query).length !== 0) { metadata.robotsNoindexNofollow = true } res.render('user/login', { diff --git a/services/web/test/acceptance/src/AddSecondaryEmailTests.mjs b/services/web/test/acceptance/src/AddSecondaryEmailTests.mjs index 4554ce2d46..5ee4989f95 100644 --- a/services/web/test/acceptance/src/AddSecondaryEmailTests.mjs +++ b/services/web/test/acceptance/src/AddSecondaryEmailTests.mjs @@ -53,6 +53,35 @@ describe('Add secondary email address confirmation code email', function () { expect(confirmCode.length).to.equal(6) }) + it('should accept an explicit null g-recaptcha-response', async function () { + // The frontend's ReCaptcha2 component (recaptcha-2.tsx) sends a literal + // JSON `null` -- not an omitted field -- whenever it never mounted the + // widget (no sitekey, action disabled, or `window.Cypress` in dev E2E + // runs). CaptchaMiddleware's schema accepts that shape rather than + // 400ing on "expected string, received null", then strips the field + // from req.body before addWithConfirmationCodeSchema -- which doesn't + // declare the field at all -- ever sees it. + const nullCaptchaRes = await user.doRequest('POST', { + json: { + email: 'secondary-null-captcha@overleaf.com', + 'g-recaptcha-response': null, + }, + uri: `/user/emails/secondary`, + }) + expect(nullCaptchaRes.response.statusCode).to.equal(200) + }) + + it('should reject a non-string g-recaptcha-response', async function () { + const badCaptchaRes = await user.doRequest('POST', { + json: { + email: 'secondary-bad-captcha@overleaf.com', + 'g-recaptcha-response': ['not', 'a', 'string'], + }, + uri: `/user/emails/secondary`, + }) + expect(badCaptchaRes.response.statusCode).to.equal(400) + }) + describe('with a valid confirmation code', function () { beforeEach(async function () { this.result = await user.doRequest('POST', { @@ -102,6 +131,16 @@ describe('Add secondary email address confirmation code email', function () { }) }) + it('should reject a non-string confirmation code', async function () { + const badCodeRes = await user.doRequest('POST', { + json: { + code: ['not', 'a', 'string'], + }, + uri: '/user/emails/confirm-secondary', + }) + expect(badCodeRes.response.statusCode).to.equal(400) + }) + describe('with a duplicate email', async function () { beforeEach(async function () { await user.doRequest('POST', { diff --git a/services/web/test/acceptance/src/TutorialTests.mjs b/services/web/test/acceptance/src/TutorialTests.mjs new file mode 100644 index 0000000000..dfb07125df --- /dev/null +++ b/services/web/test/acceptance/src/TutorialTests.mjs @@ -0,0 +1,79 @@ +import { expect } from 'chai' +import UserHelper from './helpers/User.mjs' +import { expectValidationErrorRaw } from '@overleaf/validation-tools/testUtils.js' + +const User = UserHelper.promises + +describe('Tutorial', function () { + let user + const tutorialKey = 'workbench-consent' + + beforeEach(async function () { + user = new User() + await user.login() + }) + + describe('POST /tutorial/:tutorialKey/complete', function () { + it('should mark the tutorial as completed', async function () { + const { response } = await user.doRequest('post', { + url: `/tutorial/${tutorialKey}/complete`, + json: true, + }) + expect(response.statusCode).to.equal(204) + }) + + it('should reject an unrecognized tutorial key with 404', async function () { + const { response, body } = await user.doRequest('post', { + url: '/tutorial/not-a-real-tutorial/complete', + json: true, + }) + expectValidationErrorRaw( + { statusCode: response.statusCode, body }, + 404, + 'tutorialKey' + ) + }) + }) + + describe('POST /tutorial/:tutorialKey/postpone', function () { + it('should postpone the tutorial with no body', async function () { + const { response } = await user.doRequest('post', { + url: `/tutorial/${tutorialKey}/postpone`, + json: true, + }) + expect(response.statusCode).to.equal(204) + }) + + it('should postpone the tutorial with a postponedUntil date', async function () { + const { response } = await user.doRequest('post', { + url: `/tutorial/${tutorialKey}/postpone`, + json: { postponedUntil: '2026-08-01T00:00:00.000Z' }, + }) + expect(response.statusCode).to.equal(204) + }) + + it('should reject a malformed postponedUntil value', async function () { + const { response, body } = await user.doRequest('post', { + url: `/tutorial/${tutorialKey}/postpone`, + json: { postponedUntil: 'not-a-date' }, + }) + expectValidationErrorRaw( + { statusCode: response.statusCode, body }, + 400, + 'postponedUntil' + ) + }) + + it('should reject an unrecognized tutorial key with 404', async function () { + const { response, body } = await user.doRequest('post', { + url: '/tutorial/not-a-real-tutorial/postpone', + json: true, + }) + expectValidationErrorRaw( + { statusCode: response.statusCode, body }, + 404, + 'tutorialKey' + ) + }) + }) +}) diff --git a/services/web/test/unit/src/Referal/ReferalConnect.test.mjs b/services/web/test/unit/src/Referal/ReferalConnect.test.mjs index cc70b2f420..1a7ff80031 100644 --- a/services/web/test/unit/src/Referal/ReferalConnect.test.mjs +++ b/services/web/test/unit/src/Referal/ReferalConnect.test.mjs @@ -1,4 +1,8 @@ -import { beforeEach, describe, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { + getRawReqInput, + setReqValidationModeForTests, +} from '@overleaf/validation-tools' const modulePath = new URL( '../../../../app/src/Features/Referal/ReferalConnect.mjs', import.meta.url @@ -9,6 +13,10 @@ describe('Referal connect middle wear', function () { ctx.connect = (await import(modulePath)).default }) + afterEach(function () { + setReqValidationModeForTests(null) + }) + it('should take a referal query string and put it on the session if it exists', async function (ctx) { await new Promise(resolve => { const req = { @@ -16,7 +24,35 @@ describe('Referal connect middle wear', function () { session: {}, } ctx.connect.use(req, {}, () => { - req.session.referal_id.should.equal(req.query.referal) + // case 3: raw test assertion on the fake req fixture built above + req.session.referal_id.should.equal(getRawReqInput(req).query.referal) + resolve() + }) + }) + }) + + it('should take the short-hand r referal query param and put it on the session', async function (ctx) { + await new Promise(resolve => { + const req = { + query: { r: '12345' }, + session: {}, + } + ctx.connect.use(req, {}, () => { + // case 3: raw test assertion on the fake req fixture built above + req.session.referal_id.should.equal(getRawReqInput(req).query.r) + resolve() + }) + }) + }) + + it('should tolerate unrelated query params from the request', async function (ctx) { + await new Promise(resolve => { + const req = { + query: { referal: '12345', utm_source: 'newsletter' }, + session: {}, + } + ctx.connect.use(req, {}, () => { + req.session.referal_id.should.equal('12345') resolve() }) }) @@ -42,7 +78,8 @@ describe('Referal connect middle wear', function () { session: {}, } ctx.connect.use(req, {}, () => { - req.session.referal_id.should.equal(req.query.fb_ref) + // case 3: raw test assertion on the fake req fixture built above + req.session.referal_id.should.equal(getRawReqInput(req).query.fb_ref) resolve() }) }) @@ -151,4 +188,20 @@ describe('Referal connect middle wear', function () { }) }) }) + + describe('request validation', function () { + beforeEach(function () { + setReqValidationModeForTests('enforce') + }) + + it('rejects a non-string referal query param', function (ctx) { + const req = { + query: { rm: ['fb', 't'] }, + session: {}, + } + expect(() => ctx.connect.use(req, {}, () => {})).toThrowError( + expect.objectContaining({ name: 'InvalidRequestError' }) + ) + }) + }) }) diff --git a/services/web/test/unit/src/Subscription/SubscriptionController.test.mjs b/services/web/test/unit/src/Subscription/SubscriptionController.test.mjs index 671fc403b5..b2aa97b73f 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionController.test.mjs +++ b/services/web/test/unit/src/Subscription/SubscriptionController.test.mjs @@ -1,5 +1,7 @@ -import { assert, beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, assert, beforeEach, describe, expect, it, vi } from 'vitest' +import { EventEmitter } from 'node:events' import sinon from 'sinon' +import { setReqValidationModeForTests } from '@overleaf/validation-tools' import MockRequest from '../helpers/MockRequest.mjs' import MockResponse from '../helpers/MockResponse.mjs' import SubscriptionErrors from '../../../../app/src/Features/Subscription/Errors.mjs' @@ -374,7 +376,17 @@ describe('SubscriptionController', function () { ctx.stubbedCurrencyCode = 'GBP' }) + afterEach(function () { + setReqValidationModeForTests(null) + }) + describe('successfulSubscription', function () { + beforeEach(function (ctx) { + // this route only ever reads `upgrade` from the query -- the shared + // `planCode` fixture above belongs to previewSubscription's route. + ctx.req.query = {} + }) + it('without a personal subscription', async function (ctx) { await new Promise(resolve => { ctx.SubscriptionViewModelBuilder.promises.buildUsersSubscriptionViewModel.resolves( @@ -428,10 +440,53 @@ describe('SubscriptionController', function () { ) }) }) + + it('sets isUpgrade when the upgrade query param is true', async function (ctx) { + await new Promise(resolve => { + ctx.req.query = { upgrade: 'true' } + ctx.SubscriptionViewModelBuilder.promises.buildUsersSubscriptionViewModel.resolves( + { + personalSubscription: 'foo', + } + ) + ctx.res.render = (url, variables) => { + expect(variables.isUpgrade).to.equal(true) + resolve() + } + ctx.SubscriptionController.successfulSubscription(ctx.req, ctx.res) + }) + }) + + it('tolerates an unrecognized extra query param via the fallback schema', async function (ctx) { + setReqValidationModeForTests('log') + await new Promise(resolve => { + ctx.req.query = { upgrade: 'true', promo: 'ABC' } + ctx.SubscriptionViewModelBuilder.promises.buildUsersSubscriptionViewModel.resolves( + { + personalSubscription: 'foo', + } + ) + ctx.res.render = (url, variables) => { + expect(variables.isUpgrade).to.equal(true) + resolve() + } + ctx.SubscriptionController.successfulSubscription(ctx.req, ctx.res) + }) + }) + + it('rejects a non-boolean-ish upgrade value', async function (ctx) { + ctx.req.query = { upgrade: 'sometimes' } + await expect( + ctx.SubscriptionController.successfulSubscription(ctx.req, ctx.res) + ).to.be.rejected + }) }) describe('userSubscriptionPage', function () { beforeEach(async function (ctx) { + // this route only ever reads `errorCode` from the query -- the shared + // `planCode` fixture above belongs to previewSubscription's route. + ctx.req.query = {} await new Promise((resolve, reject) => { ctx.SubscriptionViewModelBuilder.promises.buildUsersSubscriptionViewModel.resolves( { @@ -536,7 +591,7 @@ describe('SubscriptionController', function () { describe('when errorCode query param is present', function () { beforeEach(async function (ctx) { - ctx.req.query.errorCode = 'payment_failed' + ctx.req.query = { errorCode: 'payment_failed' } await new Promise((resolve, reject) => { ctx.res.render = (view, data) => { ctx.data = data @@ -555,6 +610,54 @@ describe('SubscriptionController', function () { expect(ctx.data.redirectedPaymentErrorCode).to.equal('payment_failed') }) }) + + describe('when an unrecognized query param is present', function () { + beforeEach(async function (ctx) { + setReqValidationModeForTests('log') + ctx.req.query = { errorCode: 'payment_failed', promo: 'ABC' } + await new Promise((resolve, reject) => { + ctx.res.render = (view, data) => { + ctx.data = data + resolve() + } + ctx.SubscriptionController.userSubscriptionPage( + ctx.req, + ctx.res, + ctx.rejectOnError(reject) + ) + }) + }) + + it('still renders using the raw query under the log-only rollout', function (ctx) { + expect(ctx.data.redirectedPaymentErrorCode).to.equal('payment_failed') + }) + }) + + describe('when hasSubscription query param is present', function () { + it('does not reject the request', async function (ctx) { + ctx.req.query = { hasSubscription: 'true' } + await new Promise((resolve, reject) => { + ctx.res.render = (view, data) => { + expect(view).to.equal('subscriptions/dashboard-react') + resolve() + } + ctx.SubscriptionController.userSubscriptionPage( + ctx.req, + ctx.res, + ctx.rejectOnError(reject) + ) + }) + }) + }) + + describe('when hasSubscription query param is invalid', function () { + it('rejects the request', async function (ctx) { + ctx.req.query = { hasSubscription: 'maybe' } + await expect( + ctx.SubscriptionController.userSubscriptionPage(ctx.req, ctx.res) + ).to.be.rejected + }) + }) }) describe('updateAccountEmailAddress via put', function () { @@ -753,6 +856,16 @@ describe('SubscriptionController', function () { ctx.SubscriptionHandler.promises.pauseSubscription.called ).to.equal(false) }) + + it('rejects an unrecognized extra param', async function (ctx) { + ctx.res = new MockResponse(vi) + ctx.req = new MockRequest(vi) + ctx.req.params = { pauseCycles: '3', unexpected: 'field' } + ctx.next = sinon.stub() + await expect( + ctx.SubscriptionController.pauseSubscription(ctx.req, ctx.res, ctx.next) + ).to.be.rejectedWith('Invalid request parameters') + }) }) describe('resumeSubscription', function () { @@ -933,6 +1046,53 @@ describe('SubscriptionController', function () { }) }) + describe('recurlyNotificationParser', function () { + beforeEach(function (ctx) { + ctx.RecurlyWrapper._parseXml = sinon + .stub() + .callsFake((xml, callback) => callback(null, { parsedFrom: xml })) + }) + + it('parses the streamed XML body and calls next', function (ctx) { + const fakeReq = new EventEmitter() + fakeReq.body = undefined + const next = sinon.stub() + + ctx.SubscriptionController.recurlyNotificationParser( + fakeReq, + ctx.res, + next + ) + fakeReq.emit('data', '') + fakeReq.emit('data', 'payload') + fakeReq.emit('end') + + expect(ctx.RecurlyWrapper._parseXml).to.have.been.calledWith( + 'payload' + ) + expect(fakeReq.body).to.deep.equal({ parsedFrom: 'payload' }) + expect(next).to.have.been.calledWith() + }) + + it('still parses and calls next under the log-only rollout when the body was already parsed', function (ctx) { + setReqValidationModeForTests('log') + const fakeReq = new EventEmitter() + fakeReq.body = { alreadyParsed: true } + const next = sinon.stub() + + ctx.SubscriptionController.recurlyNotificationParser( + fakeReq, + ctx.res, + next + ) + fakeReq.emit('data', 'event') + fakeReq.emit('end') + + expect(next).to.have.been.calledWith() + expect(fakeReq.body).to.deep.equal({ parsedFrom: 'event' }) + }) + }) + describe('removeAddon', function () { beforeEach(function (ctx) { ctx.SessionManager.getSessionUser.returns(ctx.user) @@ -997,6 +1157,73 @@ describe('SubscriptionController', function () { 'Cannot remove add-on while there are multiple pending subscription changes. Please contact support.', }) }) + + it('rejects an unrecognized extra param', async function (ctx) { + ctx.req.params = { addOnCode: AI_ADD_ON_CODE, unexpected: 'field' } + + await expect( + ctx.SubscriptionController.removeAddon(ctx.req, ctx.res, ctx.next) + ).to.be.rejectedWith('Invalid request parameters') + + expect(ctx.SubscriptionHandler.promises.removeAddon).to.not.have.been + .called + }) + }) + + describe('reactivateAddon', function () { + beforeEach(function (ctx) { + ctx.SessionManager.getSessionUser.returns(ctx.user) + ctx.req.params = { addOnCode: AI_ADD_ON_CODE } + ctx.SubscriptionHandler.promises.reactivateAddon = sinon.stub().resolves() + }) + + it('should return 200 on successful reactivation of AI add-on', async function (ctx) { + ctx.res.sendStatus = sinon.spy() + + await ctx.SubscriptionController.reactivateAddon(ctx.req, ctx.res) + + expect( + ctx.SubscriptionHandler.promises.reactivateAddon + ).to.have.been.calledWith(ctx.user._id, AI_ADD_ON_CODE) + expect(ctx.res.sendStatus).to.have.been.calledWith(200) + }) + + it('should return 404 if the add-on code is not AI_ADD_ON_CODE', async function (ctx) { + ctx.req.params = { addOnCode: 'some-other-addon' } + ctx.res.sendStatus = sinon.spy() + + await ctx.SubscriptionController.reactivateAddon(ctx.req, ctx.res) + + expect(ctx.SubscriptionHandler.promises.reactivateAddon).to.not.have.been + .called + expect(ctx.res.sendStatus).to.have.been.calledWith(404) + }) + + it('should handle AddOnNotPresentError and send badRequest', async function (ctx) { + ctx.SubscriptionHandler.promises.reactivateAddon.rejects( + new SubscriptionErrors.AddOnNotPresentError() + ) + + await ctx.SubscriptionController.reactivateAddon(ctx.req, ctx.res) + + expect(ctx.HttpErrorHandler.badRequest).to.have.been.calledWith( + ctx.req, + ctx.res, + 'The requested add-on is not pending cancellation', + { addon: AI_ADD_ON_CODE } + ) + }) + + it('rejects an unrecognized extra param', async function (ctx) { + ctx.req.params = { addOnCode: AI_ADD_ON_CODE, unexpected: 'field' } + + await expect( + ctx.SubscriptionController.reactivateAddon(ctx.req, ctx.res) + ).to.be.rejectedWith('Invalid request parameters') + + expect(ctx.SubscriptionHandler.promises.reactivateAddon).to.not.have.been + .called + }) }) describe('checkSubscriptionPauseStatus', function () { @@ -1711,4 +1938,175 @@ describe('SubscriptionController', function () { ) }) }) + + describe('refreshUserFeatures', function () { + beforeEach(function (ctx) { + ctx.res.sendStatus = sinon.spy() + }) + + it('should refresh features for a valid user id', async function (ctx) { + const userId = '507f1f77bcf86cd799439011' + ctx.req.params = { user_id: userId } + + await ctx.SubscriptionController.refreshUserFeatures(ctx.req, ctx.res) + + expect( + ctx.FeaturesUpdater.promises.refreshFeatures + ).to.have.been.calledWith(userId, 'acceptance-test') + expect(ctx.res.sendStatus).to.have.been.calledWith(200) + }) + + it('tolerates a malformed user id under the log-only rollout', async function (ctx) { + setReqValidationModeForTests('log') + ctx.req.params = { user_id: 'not-a-mongo-id' } + + await ctx.SubscriptionController.refreshUserFeatures(ctx.req, ctx.res) + + expect( + ctx.FeaturesUpdater.promises.refreshFeatures + ).to.have.been.calledWith('not-a-mongo-id', 'acceptance-test') + expect(ctx.res.sendStatus).to.have.been.calledWith(200) + }) + }) + + describe('getRecommendedCurrency', function () { + beforeEach(function (ctx) { + ctx.req.query = {} + }) + + it('should return the recommended currency using the request ip by default', async function (ctx) { + const result = await ctx.SubscriptionController.getRecommendedCurrency( + ctx.req, + ctx.res + ) + + expect(ctx.GeoIpLookup.promises.getCurrencyCode).to.have.been.calledWith( + ctx.req.ip + ) + expect(result).to.deep.equal({ + currency: 'USD', + recommendedCurrency: 'USD', + countryCode: 'US', + }) + }) + + it('should use an admin-supplied ip override', async function (ctx) { + ctx.AuthorizationManager.promises.isUserSiteAdmin.resolves(true) + ctx.req.query = { ip: '1.2.3.4' } + + await ctx.SubscriptionController.getRecommendedCurrency(ctx.req, ctx.res) + + expect(ctx.GeoIpLookup.promises.getCurrencyCode).to.have.been.calledWith( + '1.2.3.4' + ) + }) + + it('should ignore an ip override from a non-admin user', async function (ctx) { + ctx.AuthorizationManager.promises.isUserSiteAdmin.resolves(false) + ctx.req.query = { ip: '1.2.3.4' } + + await ctx.SubscriptionController.getRecommendedCurrency(ctx.req, ctx.res) + + expect(ctx.GeoIpLookup.promises.getCurrencyCode).to.have.been.calledWith( + ctx.req.ip + ) + }) + + it('rejects a malformed ip override', async function (ctx) { + ctx.AuthorizationManager.promises.isUserSiteAdmin.resolves(true) + ctx.req.query = { ip: 'not-an-ip' } + + await expect( + ctx.SubscriptionController.getRecommendedCurrency(ctx.req, ctx.res) + ).to.be.rejected + expect(ctx.GeoIpLookup.promises.getCurrencyCode).to.not.have.been.called + }) + + it('tolerates a non-string ip query value under the log-only rollout', async function (ctx) { + setReqValidationModeForTests('log') + ctx.AuthorizationManager.promises.isUserSiteAdmin.resolves(true) + ctx.req.query = { ip: ['1.2.3.4', '5.6.7.8'] } + + await expect( + ctx.SubscriptionController.getRecommendedCurrency(ctx.req, ctx.res) + ).to.not.be.rejected + expect(ctx.GeoIpLookup.promises.getCurrencyCode).to.have.been.called + }) + }) + + describe('getLatamCountryBannerDetails', function () { + beforeEach(function (ctx) { + ctx.req.query = {} + }) + + it('should return an empty object for a non-LATAM country', async function (ctx) { + const result = + await ctx.SubscriptionController.getLatamCountryBannerDetails( + ctx.req, + ctx.res + ) + + expect(result).to.deep.equal({}) + }) + + it('should return the Mexico banner details', async function (ctx) { + ctx.GeoIpLookup.promises.getCurrencyCode.resolves({ + countryCode: 'MX', + currencyCode: 'MXN', + }) + + const result = + await ctx.SubscriptionController.getLatamCountryBannerDetails( + ctx.req, + ctx.res + ) + + expect(result).to.deep.equal({ + latamCountryFlag: '🇲🇽', + country: 'Mexico', + discount: '25%', + currency: 'Mexican Pesos', + }) + }) + + it('should use an admin-supplied ip override', async function (ctx) { + ctx.AuthorizationManager.promises.isUserSiteAdmin.resolves(true) + ctx.req.query = { ip: '1.2.3.4' } + + await ctx.SubscriptionController.getLatamCountryBannerDetails( + ctx.req, + ctx.res + ) + + expect(ctx.GeoIpLookup.promises.getCurrencyCode).to.have.been.calledWith( + '1.2.3.4' + ) + }) + + it('rejects a malformed ip override', async function (ctx) { + ctx.AuthorizationManager.promises.isUserSiteAdmin.resolves(true) + ctx.req.query = { ip: 'not-an-ip' } + + await expect( + ctx.SubscriptionController.getLatamCountryBannerDetails( + ctx.req, + ctx.res + ) + ).to.be.rejected + expect(ctx.GeoIpLookup.promises.getCurrencyCode).to.not.have.been.called + }) + + it('tolerates a non-string ip query value under the log-only rollout', async function (ctx) { + setReqValidationModeForTests('log') + ctx.AuthorizationManager.promises.isUserSiteAdmin.resolves(true) + ctx.req.query = { ip: ['1.2.3.4'] } + + await expect( + ctx.SubscriptionController.getLatamCountryBannerDetails( + ctx.req, + ctx.res + ) + ).to.not.be.rejected + }) + }) }) diff --git a/services/web/test/unit/src/Subscription/SubscriptionGroupController.test.mjs b/services/web/test/unit/src/Subscription/SubscriptionGroupController.test.mjs index f658de272f..1fa52e9780 100644 --- a/services/web/test/unit/src/Subscription/SubscriptionGroupController.test.mjs +++ b/services/web/test/unit/src/Subscription/SubscriptionGroupController.test.mjs @@ -1,5 +1,9 @@ import { beforeEach, describe, it, vi } from 'vitest' import sinon from 'sinon' +import { + InvalidParamsError, + InvalidRequestError, +} from '@overleaf/validation-tools' const modulePath = '../../../../app/src/Features/Subscription/SubscriptionGroupController' @@ -8,7 +12,7 @@ describe('SubscriptionGroupController', function () { beforeEach(async function (ctx) { ctx.user = { _id: '!@312431', email: 'user@email.com' } ctx.adminUserId = '123jlkj' - ctx.subscriptionId = '123434325412' + ctx.subscriptionId = '123434325412341234123412' ctx.user_email = 'bob@gmail.com' ctx.req = { session: { @@ -81,6 +85,7 @@ describe('SubscriptionGroupController', function () { ctx.SubscriptionLocator = { promises: { getSubscription: sinon.stub().resolves(ctx.subscription), + getUsersSubscription: sinon.stub().resolves(ctx.subscription), }, } @@ -237,8 +242,8 @@ describe('SubscriptionGroupController', function () { describe('removeUserFromGroup', function () { it('should use the subscription id for the logged in user and take the user id from the params', async function (ctx) { await new Promise(resolve => { - const userIdToRemove = '31231' - ctx.req.params = { user_id: userIdToRemove } + const userIdToRemove = '222222222222222222222222' + ctx.req.params = { id: ctx.subscriptionId, user_id: userIdToRemove } ctx.req.entity = ctx.subscription const res = { @@ -258,8 +263,8 @@ describe('SubscriptionGroupController', function () { it('should log that the user has been removed', async function (ctx) { await new Promise(resolve => { - const userIdToRemove = '31231' - ctx.req.params = { user_id: userIdToRemove } + const userIdToRemove = '222222222222222222222222' + ctx.req.params = { id: ctx.subscriptionId, user_id: userIdToRemove } ctx.req.entity = ctx.subscription const res = { @@ -281,8 +286,8 @@ describe('SubscriptionGroupController', function () { it('should unlink the user from group SSO when they are linked, even if SSO is disabled', async function (ctx) { await new Promise(resolve => { - const userIdToRemove = '31231' - ctx.req.params = { user_id: userIdToRemove } + const userIdToRemove = '222222222222222222222222' + ctx.req.params = { id: ctx.subscriptionId, user_id: userIdToRemove } ctx.req.entity = ctx.subscription ctx.UserGetter.promises.getUser.resolves({ _id: userIdToRemove, @@ -307,8 +312,8 @@ describe('SubscriptionGroupController', function () { it('should not unlink the user when they have no group SSO link', async function (ctx) { await new Promise(resolve => { - const userIdToRemove = '31231' - ctx.req.params = { user_id: userIdToRemove } + const userIdToRemove = '222222222222222222222222' + ctx.req.params = { id: ctx.subscriptionId, user_id: userIdToRemove } ctx.req.entity = ctx.subscription ctx.UserGetter.promises.getUser.resolves({ _id: userIdToRemove, @@ -326,6 +331,18 @@ describe('SubscriptionGroupController', function () { ctx.Controller.removeUserFromGroup(ctx.req, res, resolve) }) }) + + it('rejects a malformed user id', async function (ctx) { + ctx.req.params = { id: ctx.subscriptionId, user_id: 'not-an-object-id' } + ctx.req.entity = ctx.subscription + await ctx.Controller.removeUserFromGroup( + ctx.req, + {} + ).should.be.rejectedWith(InvalidParamsError) + ctx.SubscriptionGroupHandler.promises.removeUserFromGroup.called.should.equal( + false + ) + }) }) describe('removeSelfFromGroup', function () { @@ -427,6 +444,17 @@ describe('SubscriptionGroupController', function () { ctx.Controller.removeSelfFromGroup(ctx.req, res, resolve) }) }) + + it('rejects a malformed subscriptionId', async function (ctx) { + ctx.req.query = { subscriptionId: 'not-an-object-id' } + await ctx.Controller.removeSelfFromGroup( + ctx.req, + {} + ).should.be.rejectedWith(InvalidRequestError) + ctx.SubscriptionLocator.promises.getSubscription.called.should.equal( + false + ) + }) }) describe('addSeatsToGroupSubscription', function () { @@ -571,6 +599,17 @@ describe('SubscriptionGroupController', function () { ctx.Controller.addSeatsToGroupSubscription(ctx.req, res) }) }) + + it('rejects an invalid query value', async function (ctx) { + ctx.req.query = { errorCode: { foo: 'bar' } } + await ctx.Controller.addSeatsToGroupSubscription( + ctx.req, + {} + ).should.be.rejectedWith(InvalidRequestError) + ctx.SubscriptionGroupHandler.promises.getUsersGroupSubscriptionDetails.called.should.equal( + false + ) + }) }) describe('previewAddSeatsSubscriptionChange', function () { @@ -638,6 +677,17 @@ describe('SubscriptionGroupController', function () { ctx.Controller.previewAddSeatsSubscriptionChange(ctx.req, res) }) }) + + it('rejects adding fewer than one seat', async function (ctx) { + ctx.req.body = { adding: 0 } + await ctx.Controller.previewAddSeatsSubscriptionChange( + ctx.req, + {} + ).should.be.rejectedWith(InvalidRequestError) + ctx.SubscriptionGroupHandler.promises.previewAddSeatsSubscriptionChange.called.should.equal( + false + ) + }) }) describe('createAddSeatsSubscriptionChange', function () { @@ -661,6 +711,7 @@ describe('SubscriptionGroupController', function () { it('should fail applying "add seats" change', async function (ctx) { await new Promise(resolve => { + ctx.req.body = { adding: 2 } ctx.SubscriptionGroupHandler.promises.createAddSeatsSubscriptionChange = sinon.stub().rejects() @@ -759,6 +810,17 @@ describe('SubscriptionGroupController', function () { ctx.Controller.createAddSeatsSubscriptionChange(ctx.req, res) }) }) + + it('rejects adding fewer than one seat', async function (ctx) { + ctx.req.body = { adding: 0 } + await ctx.Controller.createAddSeatsSubscriptionChange( + ctx.req, + {} + ).should.be.rejectedWith(InvalidRequestError) + ctx.SubscriptionGroupHandler.promises.createAddSeatsSubscriptionChange.called.should.equal( + false + ) + }) }) describe('submitForm', function () { @@ -828,6 +890,14 @@ describe('SubscriptionGroupController', function () { ctx.Controller.submitForm(ctx.req, res, resolve) }) }) + + it('rejects adding fewer than the minimum increase', async function (ctx) { + ctx.req.body = { adding: 5 } + await ctx.Controller.submitForm(ctx.req, {}).should.be.rejectedWith( + InvalidRequestError + ) + ctx.Modules.promises.hooks.fire.called.should.equal(false) + }) }) describe('subscriptionUpgradePage', function () { @@ -928,6 +998,17 @@ describe('SubscriptionGroupController', function () { ctx.Controller.subscriptionUpgradePage(ctx.req, res) }) }) + + it('rejects an invalid query value', async function (ctx) { + ctx.req.query = { errorCode: { foo: 'bar' } } + await ctx.Controller.subscriptionUpgradePage( + ctx.req, + {} + ).should.be.rejectedWith(InvalidRequestError) + ctx.SubscriptionGroupHandler.promises.getGroupPlanUpgradePreview.called.should.equal( + false + ) + }) }) describe('upgradeSubscription', function () { @@ -1019,4 +1100,109 @@ describe('SubscriptionGroupController', function () { }) }) }) + + describe('manuallyCollectedSubscription', function () { + it('should render the "manually collected subscription" page', async function (ctx) { + await new Promise(resolve => { + ctx.req.query = { error_type: 'no-additional-license' } + + const res = { + render: (page, data) => { + ctx.SubscriptionLocator.promises.getUsersSubscription + .calledWith(ctx.req.session.user._id) + .should.equal(true) + page.should.equal('subscriptions/manually-collected-subscription') + data.groupName.should.equal(ctx.subscription.teamName) + data.errorType.should.equal('no-additional-license') + resolve() + }, + } + + ctx.Controller.manuallyCollectedSubscription(ctx.req, res) + }) + }) + + it('should redirect to subscription page when loading the subscription fails', async function (ctx) { + await new Promise(resolve => { + ctx.SubscriptionLocator.promises.getUsersSubscription = sinon + .stub() + .rejects() + + const res = { + redirect: url => { + url.should.equal('/user/subscription') + resolve() + }, + } + + ctx.Controller.manuallyCollectedSubscription(ctx.req, res) + }) + }) + + it('rejects an invalid query value', async function (ctx) { + ctx.req.query = { error_type: { foo: 'bar' } } + await ctx.Controller.manuallyCollectedSubscription( + ctx.req, + {} + ).should.be.rejectedWith(InvalidRequestError) + ctx.SubscriptionLocator.promises.getUsersSubscription.called.should.equal( + false + ) + }) + }) + + describe('getGroupPlanPerUserPrices', function () { + it('should return the per-user prices for the requested currency', async function (ctx) { + await new Promise(resolve => { + ctx.req.query = { currency: 'USD' } + const prices = [{ USD: { collaborator: 1500 } }] + ctx.Modules.promises.hooks.fire + .withArgs( + 'getGroupPlanPerUserPrices', + ctx.req.session.user._id, + 'USD' + ) + .resolves(prices) + + const res = { + json: data => { + data.should.deep.equal(prices[0]) + resolve() + }, + } + + ctx.Controller.getGroupPlanPerUserPrices(ctx.req, res) + }) + }) + + it('should send a 500 response when the hook fails', async function (ctx) { + await new Promise(resolve => { + ctx.Modules.promises.hooks.fire + .withArgs( + 'getGroupPlanPerUserPrices', + sinon.match.any, + sinon.match.any + ) + .rejects() + + const res = { + sendStatus: code => { + code.should.equal(500) + resolve() + }, + } + + ctx.Controller.getGroupPlanPerUserPrices(ctx.req, res) + }) + }) + + it('rejects an invalid query value', async function (ctx) { + ctx.req.query = { currency: { foo: 'bar' } } + await ctx.Controller.getGroupPlanPerUserPrices( + ctx.req, + {} + ).should.be.rejectedWith(InvalidRequestError) + ctx.Modules.promises.hooks.fire.called.should.equal(false) + }) + }) }) diff --git a/services/web/test/unit/src/Subscription/TeamInvitesController.test.mjs b/services/web/test/unit/src/Subscription/TeamInvitesController.test.mjs index 14c74cf1bb..a7cfff23f4 100644 --- a/services/web/test/unit/src/Subscription/TeamInvitesController.test.mjs +++ b/services/web/test/unit/src/Subscription/TeamInvitesController.test.mjs @@ -1,5 +1,10 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import sinon from 'sinon' +import { + InvalidParamsError, + InvalidRequestError, + setReqValidationModeForTests, +} from '@overleaf/validation-tools' import MockResponse from '../helpers/MockResponse.mjs' const modulePath = @@ -146,6 +151,10 @@ describe('TeamInvitesController', function () { ctx.Controller = (await import(modulePath)).default }) + afterEach(function () { + setReqValidationModeForTests(null) + }) + describe('acceptInvite', function () { it('should add an audit log entry', async function (ctx) { await new Promise(resolve => { @@ -242,7 +251,7 @@ describe('TeamInvitesController', function () { }, } ctx.Controller.viewInvite( - { params: { token: 'token123' }, session: {} }, + { params: { token: 'token123' }, query: {}, session: {} }, res ) }) @@ -258,7 +267,7 @@ describe('TeamInvitesController', function () { }, } ctx.Controller.viewInvite( - { params: { token: 'token123' }, session: {} }, + { params: { token: 'token123' }, query: {}, session: {} }, res ) }) @@ -364,4 +373,57 @@ describe('TeamInvitesController', function () { }) }) }) + + describe('request validation', function () { + beforeEach(function () { + setReqValidationModeForTests('enforce') + }) + + it('createInvite rejects an unrecognized field in the body', async function (ctx) { + ctx.req.body = { email: 'invited@example.com', notAField: 'nope' } + await new Promise(resolve => { + ctx.Controller.createInvite(ctx.req, new MockResponse(vi), err => { + expect(err).to.be.instanceof(InvalidRequestError) + resolve() + }) + }) + }) + + it('viewInvite rejects a request missing the token param', async function (ctx) { + ctx.req.params = {} + await new Promise(resolve => { + ctx.Controller.viewInvite(ctx.req, new MockResponse(vi), err => { + expect(err).to.be.instanceof(InvalidParamsError) + resolve() + }) + }) + }) + + it('acceptInvite rejects a request missing the token param', async function (ctx) { + ctx.req.params = {} + await new Promise(resolve => { + ctx.Controller.acceptInvite(ctx.req, new MockResponse(vi), err => { + expect(err).to.be.instanceof(InvalidParamsError) + resolve() + }) + }) + }) + + it('revokeInvite rejects a malformed group id param', function (ctx) { + ctx.req.params = { id: 'not-an-object-id', email: 'user@example.com' } + expect(() => + ctx.Controller.revokeInvite(ctx.req, new MockResponse(vi), sinon.stub()) + ).to.throw(InvalidParamsError) + }) + + it('resendInvite rejects a non-string email in the body', async function (ctx) { + ctx.req.body = { email: 12345 } + await new Promise(resolve => { + ctx.Controller.resendInvite(ctx.req, new MockResponse(vi), err => { + expect(err).to.be.instanceof(InvalidRequestError) + resolve() + }) + }) + }) + }) }) diff --git a/services/web/test/unit/src/Templates/TemplatesController.test.mjs b/services/web/test/unit/src/Templates/TemplatesController.test.mjs index c2729e2164..3a159c2e72 100644 --- a/services/web/test/unit/src/Templates/TemplatesController.test.mjs +++ b/services/web/test/unit/src/Templates/TemplatesController.test.mjs @@ -49,7 +49,7 @@ describe('TemplatesController', function () { ctx.next = sinon.stub() ctx.req = { body: { - brandVariationId: 'brand-variation-id', + brandVariationId: '789', compiler: 'compiler', mainFile: 'main-file', templateId: '123', @@ -86,7 +86,7 @@ describe('TemplatesController', function () { it('should call TemplatesManager', function (ctx) { return ctx.TemplatesManager.promises.createProjectFromV1Template.should.have.been.calledWithMatch( - 'brand-variation-id', + 789, 'compiler', 'main-file', '123', @@ -145,6 +145,19 @@ describe('TemplatesController', function () { ctx.TemplatesManager.promises.createProjectFromV1Template.should.not .have.been.called }) + + it('should reject a path-traversal-shaped brandVariationId', async function (ctx) { + ctx.req.body.brandVariationId = '1/../../v1/x' + await ctx.TemplatesController.createProjectFromV1Template( + ctx.req, + ctx.res, + ctx.next + ) + ctx.next.should.have.been.calledWithMatch(sinon.match.instanceOf(Error)) + ctx.TemplatesManager.promises.createProjectFromV1Template.should.not + .have.been.called + ctx.res.redirect.called.should.equal(false) + }) }) describe('on error', function () { @@ -218,5 +231,12 @@ describe('TemplatesController', function () { ctx.next.should.have.been.calledWithMatch(sinon.match.instanceOf(Error)) ctx.res.render.called.should.equal(false) }) + + it('should reject a path-traversal-shaped brandVariationId query param', async function (ctx) { + ctx.req.query.brandVariationId = '1/../../v1/x' + await ctx.TemplatesController.getV1Template(ctx.req, ctx.res, ctx.next) + ctx.next.should.have.been.calledWithMatch(sinon.match.instanceOf(Error)) + ctx.res.render.called.should.equal(false) + }) }) }) diff --git a/services/web/test/unit/src/Tutorial/TutorialController.test.mjs b/services/web/test/unit/src/Tutorial/TutorialController.test.mjs new file mode 100644 index 0000000000..835410ab43 --- /dev/null +++ b/services/web/test/unit/src/Tutorial/TutorialController.test.mjs @@ -0,0 +1,171 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import sinon from 'sinon' +import MockRequest from '../helpers/MockRequest.mjs' +import MockResponse from '../helpers/MockResponse.mjs' +import { setReqValidationModeForTests } from '@overleaf/validation-tools' + +const modulePath = + '../../../../app/src/Features/Tutorial/TutorialController.mjs' + +describe('TutorialController', function () { + const userId = 'user-id-123' + const validKey = 'workbench-consent' + + beforeEach(async function (ctx) { + ctx.TutorialHandler = { + setTutorialState: sinon.stub().resolves(), + } + ctx.SessionManager = { + getLoggedInUserId: sinon.stub().returns(userId), + } + + vi.doMock('../../../../app/src/Features/Tutorial/TutorialHandler', () => ({ + default: ctx.TutorialHandler, + })) + vi.doMock( + '../../../../app/src/Features/Authentication/SessionManager', + () => ({ + default: ctx.SessionManager, + }) + ) + + ctx.controller = (await import(modulePath)).default + + ctx.req = new MockRequest(vi) + ctx.res = new MockResponse(vi) + ctx.next = sinon.stub() + }) + + afterEach(function () { + setReqValidationModeForTests(null) + }) + + describe('completeTutorial', function () { + it('should mark the tutorial as completed', async function (ctx) { + ctx.req.params = { tutorialKey: validKey } + await ctx.controller.completeTutorial(ctx.req, ctx.res, ctx.next) + + expect( + ctx.TutorialHandler.setTutorialState.calledWith( + userId, + validKey, + 'completed' + ) + ).to.equal(true) + expect(ctx.res.sendStatus).toHaveBeenCalledWith(204) + }) + + describe('request validation', function () { + beforeEach(function () { + setReqValidationModeForTests('enforce') + }) + + it('should reject an unrecognized tutorial key with 404', async function (ctx) { + ctx.req.params = { tutorialKey: 'not-a-real-tutorial' } + await ctx.controller.completeTutorial(ctx.req, ctx.res, ctx.next) + + expect(ctx.TutorialHandler.setTutorialState.called).to.equal(false) + expect(ctx.next.calledOnce).to.equal(true) + expect(ctx.next.firstCall.args[0].name).to.equal('InvalidParamsError') + }) + }) + + describe('with the schema in log-only mode', function () { + beforeEach(function () { + setReqValidationModeForTests('log') + }) + + it('should still reject an unrecognized tutorial key with 404', async function (ctx) { + ctx.req.params = { tutorialKey: 'not-a-real-tutorial' } + await ctx.controller.completeTutorial(ctx.req, ctx.res, ctx.next) + + expect(ctx.TutorialHandler.setTutorialState.called).to.equal(false) + expect(ctx.res.sendStatus).toHaveBeenCalledWith(404) + }) + }) + }) + + describe('postponeTutorial', function () { + it('should postpone the tutorial with no postponedUntil', async function (ctx) { + ctx.req.params = { tutorialKey: validKey } + ctx.req.body = {} + await ctx.controller.postponeTutorial(ctx.req, ctx.res, ctx.next) + + expect( + ctx.TutorialHandler.setTutorialState.calledWith( + userId, + validKey, + 'postponed', + undefined + ) + ).to.equal(true) + expect(ctx.res.sendStatus).toHaveBeenCalledWith(204) + }) + + it('should postpone the tutorial with a postponedUntil date', async function (ctx) { + ctx.req.params = { tutorialKey: validKey } + ctx.req.body = { postponedUntil: '2026-08-01T00:00:00.000Z' } + await ctx.controller.postponeTutorial(ctx.req, ctx.res, ctx.next) + + const call = ctx.TutorialHandler.setTutorialState.lastCall + expect(call.args[0]).to.equal(userId) + expect(call.args[1]).to.equal(validKey) + expect(call.args[2]).to.equal('postponed') + expect(call.args[3]).to.be.an.instanceof(Date) + expect(call.args[3].toISOString()).to.equal('2026-08-01T00:00:00.000Z') + expect(ctx.res.sendStatus).toHaveBeenCalledWith(204) + }) + + it('tolerates a non-ISO postponedUntil value via the fallback schema', async function (ctx) { + setReqValidationModeForTests('log') + ctx.req.params = { tutorialKey: validKey } + ctx.req.body = { postponedUntil: '2026/08/01' } + await ctx.controller.postponeTutorial(ctx.req, ctx.res, ctx.next) + + const call = ctx.TutorialHandler.setTutorialState.lastCall + expect(call.args[1]).to.equal(validKey) + expect(call.args[2]).to.equal('postponed') + expect(call.args[3]).to.be.an.instanceof(Date) + expect(ctx.res.sendStatus).toHaveBeenCalledWith(204) + }) + + describe('request validation', function () { + beforeEach(function () { + setReqValidationModeForTests('enforce') + }) + + it('should reject an unrecognized tutorial key with 404', async function (ctx) { + ctx.req.params = { tutorialKey: 'not-a-real-tutorial' } + ctx.req.body = {} + await ctx.controller.postponeTutorial(ctx.req, ctx.res, ctx.next) + + expect(ctx.TutorialHandler.setTutorialState.called).to.equal(false) + expect(ctx.next.calledOnce).to.equal(true) + }) + + it('should reject a malformed postponedUntil value', async function (ctx) { + ctx.req.params = { tutorialKey: validKey } + ctx.req.body = { postponedUntil: 'not-a-date' } + await ctx.controller.postponeTutorial(ctx.req, ctx.res, ctx.next) + + expect(ctx.TutorialHandler.setTutorialState.called).to.equal(false) + expect(ctx.next.calledOnce).to.equal(true) + }) + }) + + describe('with the schema in log-only mode', function () { + beforeEach(function () { + setReqValidationModeForTests('log') + }) + + it('should still reject an unrecognized tutorial key with 404', async function (ctx) { + ctx.req.params = { tutorialKey: 'not-a-real-tutorial' } + ctx.req.body = {} + await ctx.controller.postponeTutorial(ctx.req, ctx.res, ctx.next) + + expect(ctx.TutorialHandler.setTutorialState.called).to.equal(false) + expect(ctx.res.sendStatus).toHaveBeenCalledWith(404) + }) + }) + }) +}) diff --git a/services/web/test/unit/src/User/ThirdPartyIdentityManager.test.mjs b/services/web/test/unit/src/User/ThirdPartyIdentityManager.test.mjs index c589368952..1042d7e816 100644 --- a/services/web/test/unit/src/User/ThirdPartyIdentityManager.test.mjs +++ b/services/web/test/unit/src/User/ThirdPartyIdentityManager.test.mjs @@ -149,6 +149,28 @@ describe('ThirdPartyIdentityManager', function () { expect(ctx.User.findOneAndUpdate).to.not.have.been.called }) + it('should reject providerId values inherited from Object.prototype', async function (ctx) { + await expect( + ctx.ThirdPartyIdentityManager.promises.link( + ctx.userId, + '__proto__', + ctx.externalUserId, + ctx.externalData, + ctx.auditLog + ) + ).to.be.rejectedWith('Not a valid provider') + await expect( + ctx.ThirdPartyIdentityManager.promises.link( + ctx.userId, + 'constructor', + ctx.externalUserId, + ctx.externalData, + ctx.auditLog + ) + ).to.be.rejectedWith('Not a valid provider') + expect(ctx.User.findOneAndUpdate).to.not.have.been.called + }) + describe('EmailHandler', function () { beforeEach(function (ctx) { ctx.EmailHandler.promises.sendEmail.rejects(anError) @@ -224,6 +246,24 @@ describe('ThirdPartyIdentityManager', function () { expect(ctx.User.findOneAndUpdate).to.not.have.been.called }) + it('should reject providerId values inherited from Object.prototype', async function (ctx) { + await expect( + ctx.ThirdPartyIdentityManager.promises.unlink( + ctx.userId, + '__proto__', + ctx.auditLog + ) + ).to.be.rejectedWith('Not a valid provider') + await expect( + ctx.ThirdPartyIdentityManager.promises.unlink( + ctx.userId, + 'constructor', + ctx.auditLog + ) + ).to.be.rejectedWith('Not a valid provider') + expect(ctx.User.findOneAndUpdate).to.not.have.been.called + }) + describe('EmailHandler', function () { beforeEach(function (ctx) { ctx.EmailHandler.promises.sendEmail.rejects(anError) diff --git a/services/web/test/unit/src/User/UserController.test.mjs b/services/web/test/unit/src/User/UserController.test.mjs index eca0c4952d..db6417be21 100644 --- a/services/web/test/unit/src/User/UserController.test.mjs +++ b/services/web/test/unit/src/User/UserController.test.mjs @@ -1,6 +1,11 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import sinon from 'sinon' import OError from '@overleaf/o-error' +import { + InvalidParamsError, + InvalidRequestError, + setReqValidationModeForTests, +} from '@overleaf/validation-tools' import Errors from '../../../../app/src/Features/Errors/Errors.js' const modulePath = '../../../../app/src/Features/User/UserController.mjs' @@ -42,7 +47,12 @@ describe('UserController', function () { }, } - ctx.UserDeleter = { promises: { deleteUser: sinon.stub().resolves() } } + ctx.UserDeleter = { + promises: { + deleteUser: sinon.stub().resolves(), + expireDeletedUser: sinon.stub().resolves(), + }, + } ctx.UserGetter = { promises: { getUser: sinon.stub().resolves(ctx.user) }, @@ -250,6 +260,10 @@ describe('UserController', function () { ctx.callback = sinon.stub() }) + afterEach(function () { + setReqValidationModeForTests(null) + }) + describe('tryDeleteUser', function () { beforeEach(function (ctx) { ctx.req.body.password = 'wat' @@ -435,6 +449,22 @@ describe('UserController', function () { }) }) }) + + describe('request validation', function () { + beforeEach(function () { + setReqValidationModeForTests('enforce') + }) + + it('rejects an unrecognized body field', async function (ctx) { + ctx.req.body.extraField = 'nope' + await ctx.UserController.tryDeleteUser( + ctx.req, + ctx.res + ).should.be.rejectedWith(InvalidRequestError) + expect(ctx.AuthenticationManager.promises.authenticate).to.not.have.been + .called + }) + }) }) describe('updateUserSettings', function () { @@ -830,6 +860,29 @@ describe('UserController', function () { }) }) }) + + describe('request validation', function () { + it('rejects a non-string first_name', async function (ctx) { + ctx.req.body = { first_name: 12345 } + await ctx.UserController.updateUserSettings( + ctx.req, + ctx.res + ).should.be.rejectedWith(InvalidRequestError) + expect(ctx.user.save).to.not.have.been.called + }) + + it('tolerates an unrecognized body field via the fallback schema', function (ctx) { + setReqValidationModeForTests('log') + return new Promise(resolve => { + ctx.req.body = { someNewSetting: 'x' } + ctx.res.sendStatus = code => { + code.should.equal(200) + resolve() + } + ctx.UserController.updateUserSettings(ctx.req, ctx.res) + }) + }) + }) }) describe('logout', function () { @@ -918,6 +971,20 @@ describe('UserController', function () { ctx.UserController.logout(ctx.req, ctx.res) }) }) + + describe('request validation', function () { + beforeEach(function () { + setReqValidationModeForTests('enforce') + }) + + it('rejects an unrecognized body field', async function (ctx) { + ctx.req.body.extraField = 'nope' + await ctx.UserController.logout( + ctx.req, + ctx.res + ).should.be.rejectedWith(InvalidRequestError) + }) + }) }) describe('clearSessions', function () { @@ -1244,6 +1311,26 @@ describe('UserController', function () { }) }) }) + + describe('request validation', function () { + beforeEach(function () { + setReqValidationModeForTests('enforce') + }) + + it('rejects an unrecognized body field', async function (ctx) { + ctx.req.body = { + newPassword1: 'newpass', + newPassword2: 'newpass', + extraField: 'nope', + } + await ctx.UserController.changePassword( + ctx.req, + ctx.res + ).should.be.rejectedWith(InvalidRequestError) + expect(ctx.AuthenticationManager.promises.authenticate).to.not.have.been + .called + }) + }) }) describe('ensureAffiliationMiddleware', function () { @@ -1456,5 +1543,59 @@ describe('UserController', function () { expect(ctx.next).to.be.calledWith(sinon.match.instanceOf(Error)) }) }) + + describe('request validation', function () { + beforeEach(function (ctx) { + ctx.user.emails = [] + ctx.Features.hasFeature.withArgs('affiliations').returns(true) + ctx.req.query = { ensureAffiliation: true, extraField: 'nope' } + }) + + it('tolerates an unrecognized query field via the fallback schema', async function (ctx) { + setReqValidationModeForTests('log') + await ctx.UserController.ensureAffiliationMiddleware( + ctx.req, + ctx.res, + ctx.next + ) + expect(ctx.next).to.have.been.calledWith() + expect(ctx.UserGetter.promises.getUser).to.have.been.called + }) + }) + }) + + describe('expireDeletedUser', function () { + beforeEach(function (ctx) { + ctx.req.params = { userId: '507f191e810c19729de860ea' } + }) + + it('should expire the deleted user and return 204', function (ctx) { + return new Promise(resolve => { + ctx.res.sendStatus = code => { + expect(code).to.equal(204) + expect( + ctx.UserDeleter.promises.expireDeletedUser + ).to.have.been.calledWith('507f191e810c19729de860ea') + resolve() + } + ctx.UserController.expireDeletedUser(ctx.req, ctx.res, ctx.next) + }) + }) + + describe('request validation', function () { + beforeEach(function () { + setReqValidationModeForTests('enforce') + }) + + it('rejects a malformed userId', async function (ctx) { + ctx.req.params = { userId: 'not-an-object-id' } + await ctx.UserController.expireDeletedUser( + ctx.req, + ctx.res + ).should.be.rejectedWith(InvalidParamsError) + expect(ctx.UserDeleter.promises.expireDeletedUser).to.not.have.been + .called + }) + }) }) }) diff --git a/services/web/test/unit/src/User/UserEmailsController.test.mjs b/services/web/test/unit/src/User/UserEmailsController.test.mjs index 438b114da7..beeb521d33 100644 --- a/services/web/test/unit/src/User/UserEmailsController.test.mjs +++ b/services/web/test/unit/src/User/UserEmailsController.test.mjs @@ -967,4 +967,24 @@ describe('UserEmailsController', function () { ) }) }) + + describe('showConfirm', function () { + it('renders the confirm email page with the token from the query string', async function (ctx) { + ctx.req.query = { token: 'mock-confirm-token' } + await ctx.UserEmailsController.showConfirm(ctx.req, ctx.res) + expect(ctx.res.renderedTemplate).to.equal('user/confirm_email') + expect(ctx.res.renderedVariables).to.deep.equal({ + token: 'mock-confirm-token', + title: 'confirm_email', + }) + }) + + it('renders with an undefined token when none is supplied', async function (ctx) { + await ctx.UserEmailsController.showConfirm(ctx.req, ctx.res) + expect(ctx.res.renderedVariables).to.deep.equal({ + token: undefined, + title: 'confirm_email', + }) + }) + }) }) diff --git a/services/web/test/unit/src/User/UserInfoController.test.mjs b/services/web/test/unit/src/User/UserInfoController.test.mjs index 2755fa300b..7c900f2476 100644 --- a/services/web/test/unit/src/User/UserInfoController.test.mjs +++ b/services/web/test/unit/src/User/UserInfoController.test.mjs @@ -1,5 +1,6 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import sinon from 'sinon' +import { setReqValidationModeForTests } from '@overleaf/validation-tools' import MockResponse from '../helpers/MockResponse.mjs' import MockRequest from '../helpers/MockRequest.mjs' import mongodb from 'mongodb-legacy' @@ -50,6 +51,10 @@ describe('UserInfoController', function () { ctx.next = sinon.stub() }) + afterEach(function () { + setReqValidationModeForTests(null) + }) + describe('getPersonalInfo', function () { describe('when the user exists with mongo id', function () { beforeEach(function (ctx) { @@ -113,11 +118,24 @@ describe('UserInfoController', function () { ctx.user_id = 'invalid' ctx.req.params = { user_id: ctx.user_id } ctx.UserGetter.getUser = sinon.stub().callsArgWith(2, null, null) - ctx.UserInfoController.getPersonalInfo(ctx.req, ctx.res, ctx.next) }) - it('should return 400 to the client', function (ctx) { + it('should throw an InvalidParamsError (404 via handleValidationError)', function (ctx) { + setReqValidationModeForTests('enforce') + expect(() => + ctx.UserInfoController.getPersonalInfo(ctx.req, ctx.res, ctx.next) + ).to.throw() + ctx.UserGetter.getUser.called.should.equal(false) + }) + + it('should return 400 without validation enforcement', function (ctx) { + setReqValidationModeForTests('log') + expect(() => + ctx.UserInfoController.getPersonalInfo(ctx.req, ctx.res, ctx.next) + ).to.not.throw() ctx.res.statusCode.should.equal(400) + ctx.UserGetter.getUser.called.should.equal(false) + ctx.next.called.should.equal(false) }) }) }) diff --git a/services/web/test/unit/src/User/UserPagesController.test.mjs b/services/web/test/unit/src/User/UserPagesController.test.mjs index be7660d621..0fe78d0cb1 100644 --- a/services/web/test/unit/src/User/UserPagesController.test.mjs +++ b/services/web/test/unit/src/User/UserPagesController.test.mjs @@ -1,6 +1,10 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import assert from 'node:assert' import sinon from 'sinon' +import { + InvalidRequestError, + setReqValidationModeForTests, +} from '@overleaf/validation-tools' import MockResponse from '../helpers/MockResponse.mjs' import MockRequest from '../helpers/MockRequest.mjs' @@ -150,6 +154,10 @@ describe('UserPagesController', function () { ctx.res = new MockResponse(vi) }) + afterEach(function () { + setReqValidationModeForTests(null) + }) + describe('registerPage', function () { it('should render the register page', async function (ctx) { await new Promise((resolve, reject) => { @@ -240,12 +248,14 @@ describe('UserPagesController', function () { }) describe('when an explicit redirect is set via query string', function () { + const redir = '/somewhere/in/particular' + beforeEach(function (ctx) { ctx.AuthenticationController.getRedirectFromSession = sinon .stub() .returns(null) ctx.AuthenticationController.setRedirectInSession = sinon.stub() - ctx.req.query.redir = '/somewhere/in/particular' + ctx.req.query = { redir } }) it('should set a redirect', async function (ctx) { @@ -256,7 +266,7 @@ describe('UserPagesController', function () { ) expect( ctx.AuthenticationController.setRedirectInSession.lastCall.args[1] - ).to.equal(ctx.req.query.redir) + ).to.equal(redir) resolve() } ctx.UserPagesController.loginPage( @@ -267,6 +277,44 @@ describe('UserPagesController', function () { }) }) }) + + describe('request validation', function () { + beforeEach(function (ctx) { + // a bracketed query param (e.g. ?redir[foo]=bar) parses to a nested + // object, which the query schema (string | string[] values only) + // rejects + ctx.req.query = { redir: { foo: 'bar' } } + ctx.AuthenticationController.getRedirectFromSession = sinon + .stub() + .returns(null) + }) + + it('should still process the request in log mode', async function (ctx) { + setReqValidationModeForTests('log') + await new Promise((resolve, reject) => { + ctx.res.callback = () => { + expect( + ctx.AuthenticationController.setRedirectInSession + ).to.have.been.calledWith(ctx.req, { foo: 'bar' }) + resolve() + } + ctx.UserPagesController.loginPage( + ctx.req, + ctx.res, + ctx.rejectOnError(reject) + ) + }) + }) + + it('should reject the request when enforced', function (ctx) { + setReqValidationModeForTests('enforce') + expect(() => + ctx.UserPagesController.loginPage(ctx.req, ctx.res) + ).to.throw(InvalidRequestError) + expect(ctx.AuthenticationController.setRedirectInSession).to.not.have + .been.called + }) + }) }) describe('sessionsPage', function () { @@ -603,5 +651,60 @@ describe('UserPagesController', function () { }) }) }) + + describe('request validation', function () { + beforeEach(function (ctx) { + // an unrecognized query param (e.g. a tracking param on an old + // link) is rejected by the strict query schema + ctx.req.query = { + remove: 'someone@example.com', + utm_source: 'newsletter', + } + }) + + it('should still process the request in log mode', async function (ctx) { + setReqValidationModeForTests('log') + await new Promise((resolve, reject) => { + ctx.res.callback = () => { + ctx.res.renderedVariables.reconfirmationRemoveEmail.should.equal( + 'someone@example.com' + ) + resolve() + } + ctx.UserPagesController.settingsPage( + ctx.req, + ctx.res, + ctx.rejectOnError(reject) + ) + }) + }) + + it('should reject an invalid remove query value', async function (ctx) { + setReqValidationModeForTests('enforce') + ctx.req.query = { remove: { foo: 'bar' } } + await ctx.UserPagesController.settingsPage( + ctx.req, + ctx.res + ).should.be.rejectedWith(InvalidRequestError) + expect(ctx.UserGetter.promises.getUser).to.not.have.been.called + }) + }) + + describe('with an oauth-complete query param', function () { + it('accepts any string value', async function (ctx) { + ctx.req.query = { 'oauth-complete': 'github' } + await new Promise((resolve, reject) => { + ctx.res.callback = () => { + ctx.res.renderedTemplate.should.equal('user/settings') + resolve() + } + ctx.UserPagesController.settingsPage( + ctx.req, + ctx.res, + ctx.rejectOnError(reject) + ) + }) + }) + }) }) })