diff --git a/services/web/app/src/Features/Exports/ExportsController.mjs b/services/web/app/src/Features/Exports/ExportsController.mjs index 67c9e06c2d..52c33b9793 100644 --- a/services/web/app/src/Features/Exports/ExportsController.mjs +++ b/services/web/app/src/Features/Exports/ExportsController.mjs @@ -9,9 +9,34 @@ import { isTrustedConversionJobUrl } from '../V1/V1ConversionHelper.mjs' import { pipeline } from 'node:stream/promises' import { parseReq, z, zz } from '../../infrastructure/Validation.mjs' +const exportProjectSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + // opaque: v1's export API accepts and forwards this verbatim, not + // necessarily a real brand_variations.id (see ExportsTests.mjs's + // "non-numeric brand variation id" coverage) + brand_variation_id: z.string(), + }), + body: z.strictObject({ + firstName: z.string().optional(), + lastName: z.string().optional(), + // additional parameters for gallery exports + title: z.string().optional(), + description: z.string().optional(), + author: z.string().optional(), + license: z.string().optional(), + // sent as a genuine JSON boolean by the publish modal frontend + // (gallery-export.jsx posts via postJSON), not a query-string value — + // z.stringbool() would reject it, since it only accepts strings. + showSource: z.boolean().optional(), + }), +}) + async function exportProject(req, res, next) { - const { project_id: projectId, brand_variation_id: brandVariationId } = - req.params + const { params, body } = parseReq(req, exportProjectSchema, { + logOnly: true, + }) + const { project_id: projectId, brand_variation_id: brandVariationId } = params const userId = SessionManager.getLoggedInUserId(req.session) const exportParams = { project_id: projectId, @@ -19,29 +44,27 @@ async function exportProject(req, res, next) { user_id: userId, } - if (req.body) { - if (req.body.firstName) { - exportParams.first_name = req.body.firstName.trim() - } - if (req.body.lastName) { - exportParams.last_name = req.body.lastName.trim() - } - // additional parameters for gallery exports - if (req.body.title) { - exportParams.title = req.body.title.trim() - } - if (req.body.description) { - exportParams.description = req.body.description.trim() - } - if (req.body.author) { - exportParams.author = req.body.author.trim() - } - if (req.body.license) { - exportParams.license = req.body.license.trim() - } - if (req.body.showSource != null) { - exportParams.show_source = req.body.showSource - } + if (body.firstName) { + exportParams.first_name = body.firstName.trim() + } + if (body.lastName) { + exportParams.last_name = body.lastName.trim() + } + // additional parameters for gallery exports + if (body.title) { + exportParams.title = body.title.trim() + } + if (body.description) { + exportParams.description = body.description.trim() + } + if (body.author) { + exportParams.author = body.author.trim() + } + if (body.license) { + exportParams.license = body.license.trim() + } + if (body.showSource != null) { + exportParams.show_source = body.showSource } try { @@ -75,6 +98,17 @@ async function exportProject(req, res, next) { } const exportStatusSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + export_id: zz.submissionId(), + }), + query: z.object({ + token: z.string().optional(), + }), +}) +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const exportStatusFallbackSchema = z.object({ params: z.object({ export_id: zz.submissionId(), }), @@ -87,7 +121,9 @@ async function exportStatus(req, res) { const { params: { export_id: exportId }, query: { token }, - } = parseReq(req, exportStatusSchema) + } = parseReq(req, exportStatusSchema, { + fallbackSchema: exportStatusFallbackSchema, + }) if (!token && settings.exports?.requireToken) { return res.status(403).json({ export_json: { @@ -121,6 +157,18 @@ async function exportStatus(req, res) { } const exportDownloadSchema = z.object({ + params: z.strictObject({ + project_id: zz.objectId(), + export_id: zz.submissionId(), + type: z.enum(['pdf', 'zip']), + }), + query: z.object({ + token: z.string().optional(), + }), +}) +// Rollout-temporary fallback (pre-refinement schema from main); delete +// when this route's REQ_VALIDATION_MODE instrumentation is removed. +const exportDownloadFallbackSchema = z.object({ params: z.object({ export_id: zz.submissionId(), type: z.enum(['pdf', 'zip']), @@ -134,7 +182,9 @@ async function exportDownload(req, res, next) { const { params: { type, export_id: exportId }, query: { token }, - } = parseReq(req, exportDownloadSchema) + } = parseReq(req, exportDownloadSchema, { + fallbackSchema: exportDownloadFallbackSchema, + }) if (!token && settings.exports?.requireToken) { return res.sendStatus(403) } diff --git a/services/web/app/src/Features/SamlLog/SamlLogHandler.mjs b/services/web/app/src/Features/SamlLog/SamlLogHandler.mjs index c55043291a..2e26a4e4c1 100644 --- a/services/web/app/src/Features/SamlLog/SamlLogHandler.mjs +++ b/services/web/app/src/Features/SamlLog/SamlLogHandler.mjs @@ -4,16 +4,31 @@ import logger from '@overleaf/logger' import loggerSerializers from '@overleaf/logger/serializers.js' import { callbackify } from 'node:util' import Settings from '@overleaf/settings' +import { + z, + parseReq, + getRawReqInput, +} from '../../infrastructure/Validation.mjs' const ALLOWED_PATHS = Settings.saml?.logAllowList || ['/saml/'] +const samlLogBodySchema = z.object({ + body: z.object({ + email: z.string().optional(), + SAMLResponse: z.string().optional(), + }), +}) + async function log(req, data, samlAssertion) { let providerId, sessionId data = data || {} try { - const { path, query } = req + const { path } = req + // audit log stores the full raw query verbatim; not read by field name + // here (case 1: verbatim forwarding) + const { query } = getRawReqInput(req) if (!ALLOWED_PATHS.some(allowedPath => path.startsWith(allowedPath))) { return } @@ -51,12 +66,13 @@ async function log(req, data, samlAssertion) { } if (data.error) { + const { body } = parseReq(req, samlLogBodySchema, { logOnly: true }) data.body = {} - if (req.body.email) { - data.body.email = req.body.email + if (body.email) { + data.body.email = body.email } - if (req.body.SAMLResponse) { - data.body.SAMLResponse = req.body.SAMLResponse + if (body.SAMLResponse) { + data.body.SAMLResponse = body.SAMLResponse } } diff --git a/services/web/app/src/Features/Subscription/TeamInvitesController.mjs b/services/web/app/src/Features/Subscription/TeamInvitesController.mjs index 340dd138d4..5c47d92348 100644 --- a/services/web/app/src/Features/Subscription/TeamInvitesController.mjs +++ b/services/web/app/src/Features/Subscription/TeamInvitesController.mjs @@ -17,6 +17,7 @@ import { RateLimiter } from '../../infrastructure/RateLimiter.mjs' import Modules from '../../infrastructure/Modules.mjs' import UserAuditLogHandler from '../User/UserAuditLogHandler.mjs' import { sanitizeSessionUserForFrontEnd } from '../../infrastructure/FrontEndUser.mjs' +import { z, zz, parseReq } from '../../infrastructure/Validation.mjs' const rateLimiters = { resendGroupInvite: new RateLimiter('resend-group-invite', { @@ -25,15 +26,22 @@ const rateLimiters = { }), } +const createInviteSchema = z.object({ + body: z.strictObject({ + email: z.string(), + }), +}) + /** * @param {any} req * @param {any} res * @param {any} next */ async function createInvite(req, res, next) { + const { body } = parseReq(req, createInviteSchema, { logOnly: true }) const teamManagerId = SessionManager.getLoggedInUserId(req.session) const subscription = req.entity - const email = EmailHelper.parseEmail(req.body.email) + const email = EmailHelper.parseEmail(body.email) if (!email) { return res.status(422).json({ error: { @@ -75,13 +83,28 @@ async function createInvite(req, res, next) { } } +const viewInviteSchema = z.object({ + params: z.strictObject({ + token: z.string(), + }), + query: z.object({ + // rendered verbatim into a `data-type='boolean'` meta tag (Pug + // auto-escapes); the frontend, not this handler, interprets it as a + // boolean, so it is not constrained to z.stringbool() here. + expired: z.string().optional(), + }), +}) + /** * @param {any} req * @param {any} res * @param {any} next */ async function viewInvite(req, res, next) { - const { token } = req.params + const { params, query } = parseReq(req, viewInviteSchema, { + logOnly: true, + }) + const { token } = params const sessionUser = SessionManager.getSessionUser(req.session) const userId = sessionUser?._id const { invite, subscription } = @@ -150,7 +173,7 @@ async function viewInvite(req, res, next) { return res.render('subscriptions/team/invite-managed', { inviterName: invite.inviterName, inviteToken: invite.token, - expired: req.query.expired, + expired: query.expired, validationStatus: Object.fromEntries(validationStatus), currentManagedUserAdminEmail, groupSSOActive, @@ -171,7 +194,7 @@ async function viewInvite(req, res, next) { inviterName: invite.inviterName, inviteToken: invite.token, hasIndividualPaidSubscription, - expired: req.query.expired, + expired: query.expired, userRestrictions: Array.from(req.userRestrictions || []), currentManagedUserAdminEmail, groupSSOActive, @@ -216,13 +239,20 @@ async function viewInvites(req, res, next) { }) } +const acceptInviteSchema = z.object({ + params: z.strictObject({ + token: z.string(), + }), +}) + /** * @param {any} req * @param {any} res * @param {any} next */ async function acceptInvite(req, res, next) { - const { token } = req.params + const { params } = parseReq(req, acceptInviteSchema, { logOnly: true }) + const { token } = params const userId = SessionManager.getLoggedInUserId(req.session) const subscription = await TeamInvitesHandler.promises.acceptInvite( @@ -252,14 +282,26 @@ async function acceptInvite(req, res, next) { res.json({ groupSSOActive }) } +const revokeInviteSchema = z.object({ + // mounted at /manage/groups/:id/invites/:email -- `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(), + email: z.string(), + }), +}) + /** * @param {any} req * @param {any} res * @param {any} next */ function revokeInvite(req, res, next) { + const { params } = parseReq(req, revokeInviteSchema, { logOnly: true }) const subscription = req.entity - const email = EmailHelper.parseEmail(req.params.email) + const email = EmailHelper.parseEmail(params.email) const teamManagerId = SessionManager.getLoggedInUserId(req.session) if (!email) { return res.sendStatus(400) @@ -282,14 +324,21 @@ function revokeInvite(req, res, next) { ) } +const resendInviteSchema = z.object({ + body: z.strictObject({ + email: z.string(), + }), +}) + /** * @param {any} req * @param {any} res * @param {any} next */ async function resendInvite(req, res, next) { + const { body } = parseReq(req, resendInviteSchema, { logOnly: true }) const { entity: subscription } = req - const userEmail = EmailHelper.parseEmail(req.body.email) + const userEmail = EmailHelper.parseEmail(body.email) await subscription.populate('admin_id', ['email', 'first_name', 'last_name']) if (!userEmail) { diff --git a/services/web/app/src/Features/User/ThirdPartyIdentityManager.mjs b/services/web/app/src/Features/User/ThirdPartyIdentityManager.mjs index 5da2254d31..135874ea54 100644 --- a/services/web/app/src/Features/User/ThirdPartyIdentityManager.mjs +++ b/services/web/app/src/Features/User/ThirdPartyIdentityManager.mjs @@ -9,7 +9,10 @@ import { User } from '../../../../app/src/models/User.mjs' import { callbackify } from '@overleaf/promise-utils' import OError from '@overleaf/o-error' -const oauthProviders = settings.oauthProviders || {} +// Map, not a plain object: providerId is externally supplied, and a plain +// object lookup is vulnerable to prototype-pollution keys such as +// '__proto__' or 'constructor' resolving to a truthy inherited value. +const oauthProviders = new Map(Object.entries(settings.oauthProviders || {})) async function getUser(providerId, externalUserId) { if (providerId == null || externalUserId == null) { @@ -54,7 +57,7 @@ async function link( retry ) { const accountLinked = true - if (!oauthProviders[providerId]) { + if (!oauthProviders.get(providerId)) { throw new Error('Not a valid provider') } @@ -121,7 +124,7 @@ async function link( async function unlink(userId, providerId, auditLog) { const accountLinked = false - if (!oauthProviders[providerId]) { + if (!oauthProviders.get(providerId)) { throw new Error('Not a valid provider') } @@ -166,7 +169,7 @@ function _getUserQuery(providerId, externalUserId) { } function _sendSecurityAlert(accountLinked, providerId, user, userId) { - const providerName = oauthProviders[providerId].name + const providerName = oauthProviders.get(providerId).name const emailOptions = EmailOptionsHelper.linkOrUnlink( accountLinked, providerName, diff --git a/services/web/app/src/Features/UserMembership/UserMembershipController.mjs b/services/web/app/src/Features/UserMembership/UserMembershipController.mjs index 5b9b1862cc..1165768fec 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipController.mjs +++ b/services/web/app/src/Features/UserMembership/UserMembershipController.mjs @@ -14,6 +14,12 @@ import Modules from '../../infrastructure/Modules.mjs' import UserMembershipAuthorization from './UserMembershipAuthorization.mjs' import SplitTestHandler from '../SplitTests/SplitTestHandler.mjs' import _ from 'lodash' +import { z, zz, parseReq } from '../../infrastructure/Validation.mjs' + +// entity ids here may be a Mongo ObjectId-shaped subscription id (group), +// a v1 numeric institution id, or a publisher slug -- the same handlers +// serve all three entity types (see UserMembershipRouter.mjs). +const entityIdSchema = z.union([z.string(), z.number()]) async function manageGroupMembers(req, res, next) { const { entity: subscription, entityConfig } = req @@ -259,9 +265,16 @@ async function exportCsv(req, res) { csvAttachment(res, csvParser.parse(users), 'Group.csv') } +const addSchema = z.object({ + body: z.strictObject({ + email: z.string(), + }), +}) + async function add(req, res) { const { entity, entityConfig } = req - const email = EmailHelper.parseEmail(req.body.email) + const { body } = parseReq(req, addSchema, { logOnly: true }) + const email = EmailHelper.parseEmail(body.email) if (email == null) { return res.status(400).json({ error: { @@ -307,9 +320,17 @@ async function add(req, res) { res.json({ user }) } +const removeSchema = z.object({ + params: z.strictObject({ + id: entityIdSchema, + userId: zz.objectId(), + }), +}) + async function remove(req, res) { const { entity, entityConfig } = req - const { userId } = req.params + const { params } = parseReq(req, removeSchema, { logOnly: true }) + const { userId } = params if (entityConfig.readOnly) { throw new Errors.NotFoundError('Cannot remove users from entity') } @@ -347,13 +368,43 @@ async function remove(req, res) { res.sendStatus(200) } +// Unlike entityIdSchema above, this id is interpolated into +// entityConfig.pathsFor()'s redirect Location right below, and -- once +// persisted as the entity's primary key -- into Institution.mjs/ +// Publisher.mjs's fetchV1DataPromise v1 API URL. Both routes creating this +// entity (publisher/institution) pass a plain Express route param, i.e. a +// single path segment on the wire, so it must not be able to introduce an +// extra path, query or fragment separator. +const createEntityIdSchema = zz.routeSegment() + +const createSchema = z.object({ + params: z.strictObject({ + // `name` is never actually populated by the current router (neither + // `/entities/publisher/create/:id` nor `/entities/institution/create/:id` + // declares a `:name` param) but is included here, matching `new()` + // below, since it's exercised directly at the unit-test level. + name: z.string().optional(), + id: createEntityIdSchema, + }), +}) + async function create(req, res) { - const entityId = req.params.id + const { params } = parseReq(req, createSchema, { logOnly: true }) + const entityId = params.id const entityConfig = req.entityConfig await UserMembershipHandler.promises.createEntity(entityId, entityConfig) res.redirect(entityConfig.pathsFor(entityId).index) } +const newSchema = z.object({ + params: z.strictObject({ + // see the comment on createSchema above -- `name` is never populated by + // the real router, only by unit tests calling this handler directly. + name: z.string().optional(), + id: entityIdSchema, + }), +}) + export default { manageGroupMembers: expressify(manageGroupMembers), manageGroupManagers: expressify(manageGroupManagers), @@ -364,9 +415,10 @@ export default { remove: expressify(remove), exportCsv: expressify(exportCsv), new(req, res, next) { + const { params } = parseReq(req, newSchema, { logOnly: true }) res.render('user_membership/new', { - entityName: req.params.name, - entityId: req.params.id, + entityName: params.name, + entityId: params.id, }) }, create: expressify(create), diff --git a/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.mjs b/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.mjs index 0a17d61d58..80f253a278 100644 --- a/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.mjs +++ b/services/web/app/src/Features/UserMembership/UserMembershipMiddleware.mjs @@ -9,7 +9,12 @@ import EntityConfigs from './UserMembershipEntityConfigs.mjs' import Errors from '../Errors/Errors.js' import HttpErrorHandler from '../Errors/HttpErrorHandler.mjs' import TemplatesManager from '../Templates/TemplatesManager.mjs' -import { z, zz, parseReq } from '../../infrastructure/Validation.mjs' +import { + z, + zz, + parseReq, + getRawReqInput, +} from '../../infrastructure/Validation.mjs' import AdminAuthorizationHelper from '../Helpers/AdminAuthorizationHelper.mjs' const { useAdminCapabilities, useNonAdminDomainCapabilities } = @@ -286,6 +291,19 @@ const fetchEntitySchema = z.discriminatedUnion('entityName', [ PostgresIdEntitySchema, ]) +const requireGraphAccessSchema = z.object({ + query: z.object({ + resource_type: z.string().optional(), + // interpolated into req.url below to re-dispatch the request + // internally -- must not be able to introduce an extra path segment, + // query or fragment separator. + resource_id: zz.routeSegment().optional(), + }), + params: z.object({ + graph: zz.routeSegment().optional(), + }), +}) + // graphs access is an edge-case: // - the entity id is in `req.query.resource_id`. It must be set as // `req.params.id` @@ -297,7 +315,10 @@ const fetchEntitySchema = z.discriminatedUnion('entityName', [ * @param {any} next */ function requireGraphAccess(req, res, next) { - const entityName = req.query.resource_type + const { query, params } = parseReq(req, requireGraphAccessSchema, { + logOnly: true, + }) + const entityName = query.resource_type if (!entityName) { return HttpErrorHandler.notFound(req, res, 'resource_type param missing') } @@ -316,15 +337,15 @@ function requireGraphAccess(req, res, next) { } // call next router with fixed params to pass it to the correct middleware chain - const { graph } = req.params + const { graph } = params const entityRoute = entityName === 'splitTest' ? 'split-test' : entityName // all other routes go through analytics, which map undefined graph type to index (to fetch all graphs) // conversion still goes directly to v1, which can not handle an undefined graph param if (!graph?.length && entityRoute === 'conversion') { - req.url = `/graphs/conversion/index/${req.query.resource_id}` + req.url = `/graphs/conversion/index/${query.resource_id}` } else { - req.url = `/graphs/${entityRoute}/${graph}/${req.query.resource_id}` + req.url = `/graphs/${entityRoute}/${graph}/${query.resource_id}` } next('route') } @@ -356,8 +377,12 @@ function fetchPublisherFromTemplate() { ) => { if (req.template.brand.slug) { // set the id as the publisher's id as it's the entity used for access - // control - req.params.id = req.template.brand.slug + // control. The immediately-following fetchEntity() call validates this + // aliased value via fetchEntitySchema, so it isn't skipping validation + // — it just can't be validated until it's written (case 4: + // pre-validation param aliasing; parseReq() returns a decoupled copy, + // so fetchEntity()'s later parseReq() call needs the live raw field). + getRawReqInput(req).params.id = req.template.brand.slug return fetchEntity()(req, res, next) } else { return next() @@ -376,8 +401,14 @@ function requireEntity() { return next() } + // fetchEntity() (which always runs immediately before this middleware) + // already validated params.id via fetchEntitySchema; re-parsing is + // idempotent and keeps the two in sync. + const { params } = parseReq(req, fetchEntitySchema, { + logOnly: true, + }) throw new Errors.NotFoundError( - `no '${req.entityName}' entity with '${req.params.id}'` + `no '${req.entityName}' entity with '${params.id}'` ) } } @@ -396,17 +427,33 @@ function requireEntityOrCreate() { return next() } + // fetchEntity() (which always runs immediately before this middleware) + // already validated params.id via fetchEntitySchema; re-parsing is + // idempotent and keeps the two in sync. + const { params } = parseReq(req, fetchEntitySchema, { + logOnly: true, + }) if (UserMembershipAuthorization.hasAdminAccess(req)) { - res.redirect(`/entities/${req.entityName}/create/${req.params.id}`) + res.redirect(`/entities/${req.entityName}/create/${params.id}`) return } throw new Errors.NotFoundError( - `no '${req.entityName}' entity with '${req.params.id}'` + `no '${req.entityName}' entity with '${params.id}'` ) } } +const fetchV1TemplateSchema = z.object({ + // v1's own docs.doc_id column (Api::V2::TemplatesController#show's + // `find_by!(doc_id: params[:id])`) is a Postgres integer -- never a Mongo + // ObjectId or slug -- so this is spliced into fetchFromV1's `new URL(...)` + // path as a real, bounded integer, not an arbitrary route segment. + params: z.object({ + id: z.coerce.number().int().positive(), + }), +}) + // fetch the template from v1, and set it in the request function fetchV1Template() { return expressify( @@ -415,7 +462,10 @@ function fetchV1Template() { /** @type {any} */ res, /** @type {any} */ next ) => { - const templateId = req.params.id + const { params } = parseReq(req, fetchV1TemplateSchema, { + logOnly: true, + }) + const templateId = params.id const body = await TemplatesManager.promises.fetchFromV1(templateId) req.template = { id: body.id, diff --git a/services/web/modules/launchpad/app/src/LaunchpadController.mjs b/services/web/modules/launchpad/app/src/LaunchpadController.mjs index f392c4692b..7bd55d136d 100644 --- a/services/web/modules/launchpad/app/src/LaunchpadController.mjs +++ b/services/web/modules/launchpad/app/src/LaunchpadController.mjs @@ -12,6 +12,7 @@ import AuthenticationManager from '../../../../app/src/Features/Authentication/A import AuthenticationController from '../../../../app/src/Features/Authentication/AuthenticationController.mjs' import SessionManager from '../../../../app/src/Features/Authentication/SessionManager.mjs' import AdminAuthorizationHelper from '../../../../app/src/Features/Helpers/AdminAuthorizationHelper.mjs' +import { z, parseReq } from '../../../../app/src/infrastructure/Validation.mjs' const { hasAdminAccess } = AdminAuthorizationHelper @@ -77,8 +78,18 @@ async function launchpadPage(req, res) { } } +const sendTestEmailSchema = z.object({ + // no hidden _csrf field: this form has no such input (see + // views/launchpad.pug), the CSRF token travels only via the + // X-Csrf-Token header set by the JSON fetch helper. + body: z.strictObject({ + email: z.string().optional(), + }), +}) + async function sendTestEmail(req, res) { - const { email } = req.body + const { body } = parseReq(req, sendTestEmailSchema, { logOnly: true }) + const { email } = body if (!email) { logger.debug({}, 'no email address supplied') return res.status(400).json({ @@ -99,6 +110,12 @@ async function sendTestEmail(req, res) { } } +const registerExternalAuthAdminSchema = z.object({ + body: z.strictObject({ + email: z.string().optional(), + }), +}) + function registerExternalAuthAdmin(authMethod) { return expressify(async function (req, res) { if (getAuthMethod() !== authMethod) { @@ -108,7 +125,10 @@ function registerExternalAuthAdmin(authMethod) { ) return res.sendStatus(403) } - const { email } = req.body + const { body } = parseReq(req, registerExternalAuthAdminSchema, { + logOnly: true, + }) + const { email } = body if (!email) { logger.debug({ authMethod }, 'no email supplied, disallow') return res.sendStatus(400) @@ -123,7 +143,7 @@ function registerExternalAuthAdmin(authMethod) { return res.sendStatus(403) } - const body = { + const userDetails = { email, password: crypto.randomBytes(32).toString('hex'), first_name: email, @@ -137,7 +157,7 @@ function registerExternalAuthAdmin(authMethod) { let user try { - user = await UserRegistrationHandler.promises.registerNewUser(body) + user = await UserRegistrationHandler.promises.registerNewUser(userDetails) } catch (err) { OError.tag(err, 'error with registerNewUser', { email, @@ -175,9 +195,16 @@ function registerExternalAuthAdmin(authMethod) { }) } +const registerAdminSchema = z.object({ + body: z.strictObject({ + email: z.string().optional(), + password: z.string().optional(), + }), +}) + async function registerAdmin(req, res) { - const { email } = req.body - const { password } = req.body + const { body } = parseReq(req, registerAdminSchema, { logOnly: true }) + const { email, password } = body if (!email || !password) { logger.debug({}, 'must supply both email and password, disallow') return res.sendStatus(400) @@ -187,10 +214,7 @@ async function registerAdmin(req, res) { const exists = await _atLeastOneAdminExists() if (exists) { - logger.debug( - { email: req.body.email }, - 'already have at least one admin user, disallow' - ) + logger.debug({ email }, 'already have at least one admin user, disallow') return res.status(403).json({ message: { type: 'error', text: 'admin user already exists' }, }) @@ -213,9 +237,10 @@ async function registerAdmin(req, res) { .json({ message: { type: 'error', text: invalidPassword.message } }) } - const body = { email, password, analyticsId: crypto.randomUUID() } + const userDetails = { email, password, analyticsId: crypto.randomUUID() } - const user = await UserRegistrationHandler.promises.registerNewUser(body) + const user = + await UserRegistrationHandler.promises.registerNewUser(userDetails) logger.debug({ userId: user._id }, 'making user an admin') diff --git a/services/web/modules/launchpad/test/acceptance/src/LaunchpadTests.mjs b/services/web/modules/launchpad/test/acceptance/src/LaunchpadTests.mjs index 9900338983..fc1c7817ae 100644 --- a/services/web/modules/launchpad/test/acceptance/src/LaunchpadTests.mjs +++ b/services/web/modules/launchpad/test/acceptance/src/LaunchpadTests.mjs @@ -1,6 +1,7 @@ import { expect } from 'chai' import cheerio from 'cheerio' import UserHelper from '../../../../../test/acceptance/src/helpers/UserHelper.mjs' +import { expectValidationErrorRaw } from '@overleaf/validation-tools/testUtils.js' describe('Launchpad', function () { const adminEmail = 'admin@example.com' @@ -71,6 +72,30 @@ describe('Launchpad', function () { message: { type: 'error', text: 'admin user already exists' }, }) + // Reject requests carrying a field the strict body schema doesn't know + // about (schema validation runs before the admin-exists check, so this + // is rejected regardless of whether an admin already exists) + csrfToken = await user.getCsrfToken() + const unknownFieldResponse = await user.fetch('/launchpad/register_admin', { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + Accept: 'application/json', + }, + body: JSON.stringify({ + _csrf: csrfToken, + email: adminEmail + '2', + password: adminPassword + '2', + notAField: 'nope', + }), + }) + const unknownFieldBody = await unknownFieldResponse.text() + expectValidationErrorRaw( + { statusCode: unknownFieldResponse.status, body: unknownFieldBody }, + 400, + 'notAField' + ) + // Log in as this new admin user const adminUser = await UserHelper.loginUser({ email: adminEmail, @@ -82,5 +107,73 @@ describe('Launchpad', function () { // Check reversedHostName is stored expect(adminUser.user.emails[0].reversedHostname).to.equal('moc.elpmaxe') + + // Site admin can send a test email + await adminUser.getCsrfToken() + const sendTestEmailResponse = await adminUser.fetch( + '/launchpad/send_test_email', + { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + Accept: 'application/json', + }, + body: JSON.stringify({ email: 'someone@example.com' }), + } + ) + expect(sendTestEmailResponse.status).to.equal(200) + expect(await sendTestEmailResponse.json()).to.deep.equal({ + message: 'Email Sent', + }) + + // Reject requests carrying a field the strict body schema doesn't know + // about + const unknownEmailFieldResponse = await adminUser.fetch( + '/launchpad/send_test_email', + { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + Accept: 'application/json', + }, + body: JSON.stringify({ + email: 'someone@example.com', + notAField: 'nope', + }), + } + ) + const unknownEmailFieldBody = await unknownEmailFieldResponse.text() + expectValidationErrorRaw( + { + statusCode: unknownEmailFieldResponse.status, + body: unknownEmailFieldBody, + }, + 400, + 'notAField' + ) + }) + + it('should deny a non-admin user access to send a test email', async function () { + const nonAdminUser = await UserHelper.createUser() + const loggedInUser = await UserHelper.loginUser({ + email: nonAdminUser.getDefaultEmail(), + password: nonAdminUser.getDefaultPassword(), + }) + await loggedInUser.getCsrfToken() + + const response = await loggedInUser.fetch('/launchpad/send_test_email', { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + Accept: 'application/json', + }, + body: JSON.stringify({ email: 'someone@example.com' }), + }) + expect(response.status).to.equal(302) + expect(response.headers.get('location')).to.equal( + UserHelper.url( + '/restricted?from=%2Flaunchpad%2Fsend_test_email' + ).toString() + ) }) }) diff --git a/services/web/test/acceptance/src/UserMembershipAuthorizationTests.mjs b/services/web/test/acceptance/src/UserMembershipAuthorizationTests.mjs index b1f7448cf6..181e060e05 100644 --- a/services/web/test/acceptance/src/UserMembershipAuthorizationTests.mjs +++ b/services/web/test/acceptance/src/UserMembershipAuthorizationTests.mjs @@ -167,6 +167,26 @@ describe('UserMembershipAuthorization', function () { done ) }) + + it('should create the institution and redirect to its managers page', function (done) { + const newV1Id = this.institution.v1Id + 1000000 + const url = `/entities/institution/create/${newV1Id}` + async.series( + [ + this.user.login.bind(this.user), + cb => this.user.ensureAdmin(cb), + cb => this.user.ensureAdminRole('engineering', cb), + this.user.login.bind(this.user), + expectPost(this.user, url, {}, 302), + expectAccess( + this.user, + `/manage/institutions/${newV1Id}/managers`, + 200 + ), + ], + done + ) + }) }) }) @@ -225,6 +245,25 @@ describe('UserMembershipAuthorization', function () { done ) }) + + it('should create the publisher and redirect to its managers page', function (done) { + const newSlug = `new-publisher-slug-${Date.now()}` + const url = `/entities/publisher/create/${newSlug}` + async.series( + [ + cb => this.user.ensureAdmin(cb), + cb => this.user.ensureAdminRole('engineering', cb), + this.user.login.bind(this.user), + expectPost(this.user, url, {}, 302), + expectAccess( + this.user, + `/manage/publishers/${newSlug}/managers`, + 200 + ), + ], + done + ) + }) }) }) }) diff --git a/services/web/test/unit/src/Exports/ExportsController.test.mjs b/services/web/test/unit/src/Exports/ExportsController.test.mjs index 43bd3637da..769d08d08e 100644 --- a/services/web/test/unit/src/Exports/ExportsController.test.mjs +++ b/services/web/test/unit/src/Exports/ExportsController.test.mjs @@ -1,15 +1,20 @@ -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' const modulePath = new URL( '../../../../app/src/Features/Exports/ExportsController.mjs', import.meta.url ).pathname describe('ExportsController', function () { - const projectId = '123njdskj9jlk' - const userId = '123nd3ijdks' - const brandVariationId = 22 + const projectId = '507f191e810c19729de860ea' + const userId = '507f191e810c19729de860eb' + const brandVariationId = '22' const firstName = 'first' const lastName = 'last' const title = 'title' @@ -309,6 +314,11 @@ describe('ExportsController', function () { beforeEach(function (ctx) { ctx.handler.fetchExport = sinon.stub().resolves('{}') ctx.req.query = { token: 'mock-token' } + setReqValidationModeForTests('enforce') + }) + + afterEach(function () { + setReqValidationModeForTests(null) }) it('should reject an invalid encoded export_id without calling v1', async function (ctx) { @@ -317,7 +327,9 @@ describe('ExportsController', function () { export_id: '..%2f..%2f..%2fv2%2fusers', } await ctx.controller.exportStatus(ctx.req, ctx.res, ctx.next) - expect(ctx.next).to.have.been.calledWith(sinon.match.instanceOf(Error)) + expect(ctx.next).to.have.been.calledWith( + sinon.match.instanceOf(InvalidParamsError) + ) expect(ctx.handler.fetchExport).not.to.have.been.called }) @@ -327,7 +339,9 @@ describe('ExportsController', function () { export_id: '../../../etc/passwd', } await ctx.controller.exportStatus(ctx.req, ctx.res, ctx.next) - expect(ctx.next).to.have.been.calledWith(sinon.match.instanceOf(Error)) + expect(ctx.next).to.have.been.calledWith( + sinon.match.instanceOf(InvalidParamsError) + ) expect(ctx.handler.fetchExport).not.to.have.been.called }) @@ -411,4 +425,76 @@ describe('ExportsController', function () { }) }) }) + + describe('request validation', function () { + beforeEach(function () { + setReqValidationModeForTests('enforce') + }) + + afterEach(function () { + setReqValidationModeForTests(null) + }) + + describe('exportProject', function () { + it('should reject a malformed project_id', async function (ctx) { + ctx.req.params.project_id = 'not-an-object-id' + await ctx.controller.exportProject(ctx.req, ctx.res, ctx.next) + expect(ctx.next).to.have.been.calledWith( + sinon.match.instanceOf(InvalidParamsError) + ) + }) + + it('should reject an unrecognized body field', async function (ctx) { + ctx.req.body.extra = 'nope' + await ctx.controller.exportProject(ctx.req, ctx.res, ctx.next) + expect(ctx.next).to.have.been.calledWith( + sinon.match.instanceOf(InvalidRequestError) + ) + }) + }) + + describe('exportStatus', function () { + beforeEach(function (ctx) { + ctx.req.query = { token: 'mock-token' } + }) + + it('should reject a malformed project_id', async function (ctx) { + ctx.req.params = { + project_id: 'not-an-object-id', + export_id: '897', + } + await ctx.controller.exportStatus(ctx.req, ctx.res, ctx.next) + expect(ctx.next).to.have.been.calledWith( + sinon.match.instanceOf(InvalidParamsError) + ) + }) + }) + + describe('exportDownload', function () { + beforeEach(function (ctx) { + ctx.req.params = { + project_id: projectId, + export_id: '897', + type: 'zip', + } + ctx.req.query = { token: 'mock-token' } + }) + + it('should reject a malformed project_id', async function (ctx) { + ctx.req.params.project_id = 'not-an-object-id' + await ctx.controller.exportDownload(ctx.req, ctx.res, ctx.next) + expect(ctx.next).to.have.been.calledWith( + sinon.match.instanceOf(InvalidParamsError) + ) + }) + + it('should reject an invalid token query value', async function (ctx) { + ctx.req.query.token = { foo: 'bar' } + await ctx.controller.exportDownload(ctx.req, ctx.res, ctx.next) + expect(ctx.next).to.have.been.calledWith( + sinon.match.instanceOf(InvalidRequestError) + ) + }) + }) + }) }) diff --git a/services/web/test/unit/src/SamlLog/SamlLogHandler.test.mjs b/services/web/test/unit/src/SamlLog/SamlLogHandler.test.mjs index f3c41d590c..4d33b970f1 100644 --- a/services/web/test/unit/src/SamlLog/SamlLogHandler.test.mjs +++ b/services/web/test/unit/src/SamlLog/SamlLogHandler.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' const modulePath = '../../../../app/src/Features/SamlLog/SamlLogHandler.mjs' @@ -163,4 +164,67 @@ describe('SamlLogHandler', function () { ctx.logger.error.should.not.have.been.called }) }) + + describe('with an error and a saml request body', function () { + function makeReq(body) { + return { + session: { saml: { providerId } }, + sessionID: sessionId, + path: '/saml/ukamf', + logger: { addFields: sinon.stub() }, + body, + } + } + + afterEach(function () { + setReqValidationModeForTests(null) + }) + + it('should log the email and SAMLResponse fields', async function () { + await SamlLogHandler.promises.log( + makeReq({ + email: 'user@example.com', + SAMLResponse: 'encoded-response', + }), + { error: new Error('boom') } + ) + + const loggedData = JSON.parse(samlLog.jsonData) + expect(loggedData.body).to.deep.equal({ + email: 'user@example.com', + SAMLResponse: 'encoded-response', + }) + samlLog.save.should.have.been.calledOnce + }) + + it('should still log by default when the body fails schema validation', async function (ctx) { + setReqValidationModeForTests('log') + await SamlLogHandler.promises.log( + makeReq({ email: { nested: 'not-a-string' } }), + { error: new Error('boom') } + ) + + const loggedData = JSON.parse(samlLog.jsonData) + expect(loggedData.body).to.deep.equal({ + email: { nested: 'not-a-string' }, + }) + samlLog.save.should.have.been.calledOnce + ctx.logger.error.should.not.have.been.called + }) + + it('should not save and should log an error when REQ_VALIDATION_MODE=enforce', async function (ctx) { + setReqValidationModeForTests('enforce') + + await SamlLogHandler.promises.log( + makeReq({ email: { nested: 'not-a-string' } }), + { error: new Error('boom') } + ) + + samlLog.save.should.not.have.been.called + ctx.logger.error.should.have.been.calledOnce.and.calledWithMatch( + { providerId, sessionId: sessionId.substr(0, 8) }, + 'SamlLog Error' + ) + }) + }) }) diff --git a/services/web/test/unit/src/UserMembership/UserMembershipController.test.mjs b/services/web/test/unit/src/UserMembership/UserMembershipController.test.mjs index 22aee221ec..41fabb090d 100644 --- a/services/web/test/unit/src/UserMembership/UserMembershipController.test.mjs +++ b/services/web/test/unit/src/UserMembership/UserMembershipController.test.mjs @@ -1,4 +1,9 @@ -import { expect, vi, describe, it, beforeEach } from 'vitest' +import { expect, vi, describe, it, beforeEach, afterEach } from 'vitest' +import { + InvalidParamsError, + InvalidRequestError, + setReqValidationModeForTests, +} from '@overleaf/validation-tools' import MockRequest from '../helpers/MockRequest.mjs' import MockResponse from '../helpers/MockResponse.mjs' import EntityConfigs from '../../../../app/src/Features/UserMembership/UserMembershipEntityConfigs.mjs' @@ -20,16 +25,21 @@ vi.mock('../../../../app/src/Features/Errors/Errors.js', () => vi.importActual('../../../../app/src/Features/Errors/Errors.js') ) +const entityId = 'mock-entity-id' + describe('UserMembershipController', () => { let recurlySubscriptionId beforeEach(async ctx => { recurlySubscriptionId = 'mock-recurly-subscription-id' ctx.req = new MockRequest(vi) - ctx.req.params.id = 'mock-entity-id' + ctx.req.params = { id: entityId } ctx.req.ip = '1.2.3.4' - ctx.user = { _id: 'mock-user-id' } - ctx.newUser = { _id: 'mock-new-user-id', email: 'new-user-email@foo.bar' } + ctx.user = { _id: '507f191e810c19729de860ea' } + ctx.newUser = { + _id: '507f191e810c19729de860eb', + email: 'new-user-email@foo.bar', + } ctx.subscription = { _id: 'mock-subscription-id', admin_id: 'mock-admin-id', @@ -241,6 +251,10 @@ describe('UserMembershipController', () => { ctx.UserMembershipController = (await import(modulePath)).default }) + afterEach(() => { + setReqValidationModeForTests(null) + }) + describe('index', () => { beforeEach(ctx => { ctx.req.user = ctx.user @@ -358,7 +372,9 @@ describe('UserMembershipController', () => { SessionManager, }) => { expect.assertions(1) - SessionManager.getLoggedInUserId.mockReturnValue('mock-user-id') + SessionManager.getLoggedInUserId.mockReturnValue( + '507f191e810c19729de860ea' + ) await UserMembershipController.manageGroupMembers(req, { render: (viewPath, viewParams) => { expect(viewParams.canUseAddSeatsFeature).to.equal(false) @@ -470,7 +486,7 @@ describe('UserMembershipController', () => { describe('add', () => { beforeEach(ctx => { - ctx.req.body.email = ctx.newUser.email + ctx.req.body = { email: ctx.newUser.email } ctx.req.entity = ctx.subscription ctx.req.entityConfig = EntityConfigs.groupManagers }) @@ -500,7 +516,7 @@ describe('UserMembershipController', () => { }, }, newUser.email, - { initiatorId: 'mock-user-id', ipAddress: '1.2.3.4' } + { initiatorId: '507f191e810c19729de860ea', ipAddress: '1.2.3.4' } ) }, }) @@ -571,7 +587,7 @@ describe('UserMembershipController', () => { it('handle invalid email', async ({ UserMembershipController, req }) => { expect.assertions(1) - req.body.email = 'not_valid_email' + req.body = { email: 'not_valid_email' } await UserMembershipController.add(req, { status: () => ({ json: payload => { @@ -580,11 +596,27 @@ describe('UserMembershipController', () => { }), }) }) + + describe('request validation', () => { + beforeEach(() => { + setReqValidationModeForTests('enforce') + }) + + it('rejects a body with an unrecognized field', async ({ + UserMembershipController, + req, + }) => { + req.body = { email: 'user@example.com', extra: 'nope' } + await UserMembershipController.add(req, {}).should.be.rejectedWith( + InvalidRequestError + ) + }) + }) }) describe('remove', () => { beforeEach(ctx => { - ctx.req.params.userId = ctx.newUser._id + ctx.req.params = { id: entityId, userId: ctx.newUser._id } ctx.req.entity = ctx.subscription ctx.req.entityConfig = EntityConfigs.groupManagers }) @@ -619,7 +651,7 @@ describe('UserMembershipController', () => { }, }, newUser._id, - { initiatorId: 'mock-user-id', ipAddress: '1.2.3.4' } + { initiatorId: '507f191e810c19729de860ea', ipAddress: '1.2.3.4' } ) }, }) @@ -640,7 +672,7 @@ describe('UserMembershipController', () => { user, }) => { expect.assertions(1) - req.params.userId = user._id + req.params = { id: entityId, userId: user._id } await UserMembershipController.remove(req, { status: () => ({ json: payload => { @@ -667,6 +699,22 @@ describe('UserMembershipController', () => { }), }) }) + + describe('request validation', () => { + beforeEach(() => { + setReqValidationModeForTests('enforce') + }) + + it('rejects a malformed userId', async ({ + UserMembershipController, + req, + }) => { + req.params = { id: entityId, userId: 'not-an-object-id' } + await UserMembershipController.remove(req, {}).should.be.rejectedWith( + InvalidParamsError + ) + }) + }) }) describe('exportCsv', () => { @@ -772,8 +820,7 @@ describe('UserMembershipController', () => { describe('new', () => { beforeEach(ctx => { - ctx.req.params.name = 'publisher' - ctx.req.params.id = 'abc' + ctx.req.params = { name: 'publisher', id: 'abc' } }) it('renders view', async ({ UserMembershipController, req }) => { @@ -785,13 +832,29 @@ describe('UserMembershipController', () => { }, }) }) + + describe('request validation', () => { + beforeEach(() => { + setReqValidationModeForTests('enforce') + }) + + it('rejects params with an unrecognized field', ({ + UserMembershipController, + req, + }) => { + req.params = { name: 'publisher', id: 'abc', extra: 'nope' } + expect(() => UserMembershipController.new(req, {})).to.throw( + InvalidParamsError + ) + }) + }) }) describe('create', () => { beforeEach(ctx => { - ctx.req.params.name = 'institution' + // Express route params are always strings on the wire. + ctx.req.params = { name: 'institution', id: '123' } ctx.req.entityConfig = EntityConfigs.institution - ctx.req.params.id = 123 }) it('creates institution', async ({ @@ -802,10 +865,10 @@ describe('UserMembershipController', () => { expect.assertions(2) await UserMembershipController.create(req, { redirect: path => { - expect(path).to.eq(EntityConfigs.institution.pathsFor(123).index) + expect(path).to.eq(EntityConfigs.institution.pathsFor('123').index) expect( UserMembershipHandler.promises.createEntity - ).toHaveBeenCalledWith(123, { + ).toHaveBeenCalledWith('123', { fields: { access: 'managerIds', membership: 'member_ids', @@ -820,5 +883,42 @@ describe('UserMembershipController', () => { }, }) }) + + describe('request validation', () => { + beforeEach(() => { + setReqValidationModeForTests('enforce') + }) + + it('rejects params with an unrecognized field', async ({ + UserMembershipController, + req, + }) => { + req.params = { name: 'institution', id: '123', extra: 'nope' } + await UserMembershipController.create(req, {}).should.be.rejectedWith( + InvalidParamsError + ) + }) + + it('accepts a plain route-segment id', async ({ + UserMembershipController, + req, + }) => { + req.params = { name: 'institution', id: '123' } + await UserMembershipController.create(req, { + redirect: () => {}, + }) + }) + + it('rejects a path-traversal id', async ({ + UserMembershipController, + req, + }) => { + req.params = { name: 'publisher', id: '../../admin' } + req.entityConfig = EntityConfigs.publisher + await UserMembershipController.create(req, {}).should.be.rejectedWith( + InvalidParamsError + ) + }) + }) }) })