From dfe587f297afdcd894b4bdff92fb0bb6bf53f9b6 Mon Sep 17 00:00:00 2001 From: Jakob Ackermann Date: Wed, 15 May 2024 15:28:53 +0200 Subject: [PATCH] Merge pull request #18294 from overleaf/jpa-td-invite-details [web] avoid content reflection via query parameter on register page GitOrigin-RevId: 43e7ba6069e0d9f3f12e5e9e680b5960b0673782 --- .../AuthenticationController.js | 2 +- .../CollaboratorsEmailHandler.js | 8 +- .../CollaboratorsInviteController.js | 38 ++++-- .../Collaborators/CollaboratorsRouter.js | 6 +- .../Features/Project/ProjectListController.js | 16 ++- .../src/Features/User/UserPagesController.js | 5 +- .../test/acceptance/src/ProjectInviteTests.js | 109 ++++++++---------- .../web/test/acceptance/src/helpers/User.js | 2 +- .../AuthenticationControllerTests.js | 5 +- .../CollaboratorsInviteControllerTests.js | 35 ++++++ .../test/unit/src/Email/EmailBuilderTests.js | 7 +- .../unit/src/User/UserPagesControllerTests.js | 6 +- 12 files changed, 143 insertions(+), 96 deletions(-) diff --git a/services/web/app/src/Features/Authentication/AuthenticationController.js b/services/web/app/src/Features/Authentication/AuthenticationController.js index af24dfec28..201862cd12 100644 --- a/services/web/app/src/Features/Authentication/AuthenticationController.js +++ b/services/web/app/src/Features/Authentication/AuthenticationController.js @@ -522,7 +522,7 @@ const AuthenticationController = { _redirectToLoginOrRegisterPage(req, res) { if ( req.query.zipUrl != null || - req.query.project_name != null || + req.session.sharedProjectData || req.path === '/user/subscription/new' ) { AuthenticationController._redirectToRegisterPage(req, res) diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsEmailHandler.js b/services/web/app/src/Features/Collaborators/CollaboratorsEmailHandler.js index 226a775134..1260a81766 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsEmailHandler.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsEmailHandler.js @@ -5,13 +5,7 @@ const Settings = require('@overleaf/settings') const CollaboratorsEmailHandler = { _buildInviteUrl(project, invite) { - return ( - `${Settings.siteUrl}/project/${project._id}/invite/token/${invite.token}?` + - [ - `project_name=${encodeURIComponent(project.name)}`, - `user_first_name=${encodeURIComponent(project.owner_ref.first_name)}`, - ].join('&') - ) + return `${Settings.siteUrl}/project/${project._id}/invite/token/${invite.token}` }, async notifyUserOfProjectInvite(projectId, email, invite, sendingUser) { diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsInviteController.js b/services/web/app/src/Features/Collaborators/CollaboratorsInviteController.js index cc457d7309..17b3169b52 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsInviteController.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsInviteController.js @@ -14,6 +14,7 @@ const { RateLimiter } = require('../../infrastructure/RateLimiter') const { expressify } = require('@overleaf/promise-utils') const ProjectAuditLogHandler = require('../Project/ProjectAuditLogHandler') const Errors = require('../Errors/Errors') +const AuthenticationController = require('../Authentication/AuthenticationController') // This rate limiter allows a different number of requests depending on the // number of callaborators a user is allowed. This is implemented by providing @@ -234,23 +235,26 @@ const CollaboratorsInviteController = { const projectId = req.params.Project_id const { token } = req.params const _renderInvalidPage = function () { + res.status(404) logger.debug({ projectId }, 'invite not valid, rendering not-valid page') res.render('project/invite/not-valid', { title: 'Invalid Invite' }) } // check if the user is already a member of the project const currentUser = SessionManager.getSessionUser(req.session) - const isMember = - await CollaboratorsGetter.promises.isUserInvitedMemberOfProject( - currentUser._id, - projectId - ) - if (isMember) { - logger.debug( - { projectId, userId: currentUser._id }, - 'user is already a member of this project, redirecting' - ) - return res.redirect(`/project/${projectId}`) + if (currentUser) { + const isMember = + await CollaboratorsGetter.promises.isUserInvitedMemberOfProject( + currentUser._id, + projectId + ) + if (isMember) { + logger.debug( + { projectId, userId: currentUser._id }, + 'user is already a member of this project, redirecting' + ) + return res.redirect(`/project/${projectId}`) + } } // get the invite @@ -284,6 +288,18 @@ const CollaboratorsInviteController = { return _renderInvalidPage() } + if (!currentUser) { + req.session.sharedProjectData = { + project_name: project.name, + user_first_name: owner.first_name, + } + AuthenticationController.setRedirectInSession(req) + return res.redirect('/register') + } + + // cleanup if set for register page + delete req.session.sharedProjectData + // finally render the invite res.render('project/invite/show', { invite, diff --git a/services/web/app/src/Features/Collaborators/CollaboratorsRouter.js b/services/web/app/src/Features/Collaborators/CollaboratorsRouter.js index 9a66bc0144..b00c1b717c 100644 --- a/services/web/app/src/Features/Collaborators/CollaboratorsRouter.js +++ b/services/web/app/src/Features/Collaborators/CollaboratorsRouter.js @@ -26,6 +26,10 @@ const rateLimiters = { points: 200, duration: 60 * 10, }), + viewProjectInvite: new RateLimiter('view-project-invite', { + points: 20, + duration: 60, + }), } module.exports = { @@ -128,7 +132,7 @@ module.exports = { 'collaboration', 'project-invite' ), - AuthenticationController.requireLogin(), + RateLimiterMiddleware.rateLimit(rateLimiters.viewProjectInvite), CollaboratorsInviteController.viewInvite, AnalyticsRegistrationSourceMiddleware.clearSource() ) diff --git a/services/web/app/src/Features/Project/ProjectListController.js b/services/web/app/src/Features/Project/ProjectListController.js index a3c7c5fe05..e4e50f73d7 100644 --- a/services/web/app/src/Features/Project/ProjectListController.js +++ b/services/web/app/src/Features/Project/ProjectListController.js @@ -80,6 +80,17 @@ const _buildPortalTemplatesList = affiliations => { return portalTemplates } +function cleanupSession(req) { + // cleanup redirects at the end of the redirect chain + delete req.session.postCheckoutRedirect + delete req.session.postLoginRedirect + delete req.session.postOnboardingRedirect + + // cleanup details from register page + delete req.session.sharedProjectData + delete req.session.templateData +} + /** * @param {import("express").Request} req * @param {import("express").Response} res @@ -87,10 +98,7 @@ const _buildPortalTemplatesList = affiliations => { * @returns {Promise} */ async function projectListPage(req, res, next) { - // cleanup redirects at the end of the redirect chain - delete req.session.postCheckoutRedirect - delete req.session.postLoginRedirect - delete req.session.postOnboardingRedirect + cleanupSession(req) // can have two values: // - undefined - when there's no "saas" feature or couldn't get subscription data diff --git a/services/web/app/src/Features/User/UserPagesController.js b/services/web/app/src/Features/User/UserPagesController.js index d582bbfdcd..54a3789f55 100644 --- a/services/web/app/src/Features/User/UserPagesController.js +++ b/services/web/app/src/Features/User/UserPagesController.js @@ -211,10 +211,7 @@ const UserPagesController = { accountSuspended: expressify(accountSuspended), registerPage(req, res) { - const sharedProjectData = { - project_name: req.query.project_name, - user_first_name: req.query.user_first_name, - } + const sharedProjectData = req.session.sharedProjectData || {} const newTemplateData = {} if (req.session.templateData != null) { diff --git a/services/web/test/acceptance/src/ProjectInviteTests.js b/services/web/test/acceptance/src/ProjectInviteTests.js index 3e28dd4da9..a5a5066d78 100644 --- a/services/web/test/acceptance/src/ProjectInviteTests.js +++ b/services/web/test/acceptance/src/ProjectInviteTests.js @@ -4,6 +4,7 @@ const User = require('./helpers/User') const settings = require('@overleaf/settings') const CollaboratorsEmailHandler = require('../../../app/src/Features/Collaborators/CollaboratorsEmailHandler') const Features = require('../../../app/src/infrastructure/Features') +const cheerio = require('cheerio') const createInvite = (sendingUser, projectId, email, callback) => { sendingUser.getCsrfToken(err => { @@ -107,24 +108,6 @@ const tryAcceptInvite = (user, invite, callback) => { }) } -const tryRegisterUser = (user, email, callback) => { - user.getCsrfToken(error => { - if (error != null) { - return callback(error) - } - user.request.post( - { - url: '/register', - json: { - email, - password: 'some_weird_password', - }, - }, - callback - ) - }) -} - const tryFollowLoginLink = (user, loginLink, callback) => { user.getCsrfToken(error => { if (error != null) { @@ -220,7 +203,7 @@ const expectInvalidInvitePage = (user, link, callback) => { // view invalid invite tryFollowInviteLink(user, link, (err, response, body) => { expect(err).not.to.exist - expect(response.statusCode).to.equal(200) + expect(response.statusCode).to.equal(404) expect(body).to.match(/Invalid Invite - .*<\/title>/) callback() }) @@ -232,7 +215,15 @@ const expectInviteRedirectToRegister = (user, link, callback) => { expect(err).not.to.exist expect(response.statusCode).to.equal(302) expect(response.headers.location).to.match(/^\/register.*$/) - callback() + + user.getSession((err, session) => { + if (err) return callback(err) + expect(session.sharedProjectData).deep.equals({ + project_name: PROJECT_NAME, + user_first_name: OWNER_NAME, + }) + callback() + }) }) } @@ -253,11 +244,26 @@ const expectLoginRedirectToInvite = (user, link, callback) => { }) } -const expectRegistrationRedirectToInvite = (user, email, link, callback) => { - tryRegisterUser(user, email, (err, response) => { +const expectRegistrationRedirectToInvite = (user, link, callback) => { + user.register((err, _user, response) => { expect(err).not.to.exist expect(response.statusCode).to.equal(200) - callback() + + if (response.body.redir === '/registration/try-premium') { + user.request.get('/registration/onboarding', (err, response) => { + if (err) return callback(err) + expect(response.statusCode).to.equal(200) + const dom = cheerio.load(response.body) + const skipUrl = dom('meta[name="ol-skipUrl"]')[0].attribs.content + expect(new URL(skipUrl, settings.siteUrl).href).to.equal( + new URL(link, settings.siteUrl).href + ) + callback() + }) + } else { + expect(response.body.redir).to.equal(link) + callback() + } }) } @@ -301,19 +307,26 @@ const expectInvitesInJoinProjectCount = (user, projectId, count, callback) => { }) } -// eslint-disable-next-line mocha/no-skipped-tests -describe.skip('ProjectInviteTests', function () { +const PROJECT_NAME = 'project name for sharing test' +const OWNER_NAME = 'sending user name' + +describe('ProjectInviteTests', function () { beforeEach(function (done) { this.sendingUser = new User() this.user = new User() - this.site_admin = new User({ email: 'admin@example.com' }) - this.email = 'smoketestuser@example.com' - this.projectName = 'sharing test' + this.site_admin = new User({ email: `admin+${Math.random()}@example.com` }) + this.email = `smoketestuser+${Math.random()}@example.com` Async.series( [ - cb => this.user.ensureUserExists(cb), cb => this.sendingUser.login(cb), cb => this.sendingUser.setFeatures({ collaborators: 10 }, cb), + cb => + this.sendingUser.mongoUpdate( + { + $set: { first_name: OWNER_NAME }, + }, + cb + ), cb => this.sendingUser.setFeaturesOverride( { @@ -328,15 +341,11 @@ describe.skip('ProjectInviteTests', function () { }) describe('creating invites', function () { - beforeEach(function () { - this.projectName = 'wat' - }) - describe('creating two invites', function () { beforeEach(function (done) { createProject( this.sendingUser, - this.projectName, + PROJECT_NAME, (err, projectId, project) => { expect(err).not.to.exist this.projectId = projectId @@ -504,7 +513,7 @@ describe.skip('ProjectInviteTests', function () { beforeEach(function (done) { createProjectAndInvite( this.sendingUser, - this.projectName, + PROJECT_NAME, this.email, (err, project, invite, link) => { expect(err).not.to.exist @@ -657,12 +666,7 @@ describe.skip('ProjectInviteTests', function () { [ cb => expectInviteRedirectToRegister(this.user, this.link, cb), cb => - expectRegistrationRedirectToInvite( - this.user, - 'some_email@example.com', - this.link, - cb - ), + expectRegistrationRedirectToInvite(this.user, this.link, cb), cb => expectInvitePage(this.user, this.link, cb), cb => expectAcceptInviteAndRedirect(this.user, this.invite, cb), cb => expectProjectAccess(this.user, this.invite.projectId, cb), @@ -689,21 +693,13 @@ describe.skip('ProjectInviteTests', function () { ) }) - it('should display invalid-invite if the user registers a new account', function (done) { + it('should display invalid-invite right away', function (done) { const badLink = this.link.replace( this.invite.token, 'not_a_real_token' ) Async.series( [ - cb => expectInviteRedirectToRegister(this.user, badLink, cb), - cb => - expectRegistrationRedirectToInvite( - this.user, - 'some_email@example.com', - badLink, - cb - ), cb => expectInvalidInvitePage(this.user, badLink, cb), cb => expectNoProjectAccess(this.user, this.invite.projectId, cb), ], @@ -713,6 +709,10 @@ describe.skip('ProjectInviteTests', function () { }) describe('login workflow with valid token', function () { + beforeEach(function (done) { + this.user.ensureUserExists(done) + }) + it('should redirect to the register page', function (done) { Async.series( [ @@ -762,20 +762,13 @@ describe.skip('ProjectInviteTests', function () { ) }) - it('should show the invalid-invite page once the user has logged in', function (done) { + it('should show the invalid-invite page right away', function (done) { const badLink = this.link.replace( this.invite.token, 'not_a_real_token' ) Async.series( [ - cb => { - return expectInviteRedirectToRegister(this.user, badLink, cb) - }, - cb => { - return expectLoginPage(this.user, cb) - }, - cb => expectLoginRedirectToInvite(this.user, badLink, cb), cb => expectInvalidInvitePage(this.user, badLink, cb), cb => expectNoProjectAccess(this.user, this.invite.projectId, cb), ], diff --git a/services/web/test/acceptance/src/helpers/User.js b/services/web/test/acceptance/src/helpers/User.js index cde2f7807d..f90c40df88 100644 --- a/services/web/test/acceptance/src/helpers/User.js +++ b/services/web/test/acceptance/src/helpers/User.js @@ -336,7 +336,7 @@ class User { return callback(error) } this.setExtraAttributes(user) - callback(null, user) + callback(null, user, response) }) } ) diff --git a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js index 3d149e8738..b709f62a6a 100644 --- a/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js +++ b/services/web/test/unit/src/Authentication/AuthenticationControllerTests.js @@ -906,7 +906,10 @@ describe('AuthenticationController', function () { describe('they have been invited to a project', function () { beforeEach(function () { - this.req.query.project_name = 'something' + this.req.session.sharedProjectData = { + project_name: 'something', + user_first_name: 'else', + } this.SessionManager.isUserLoggedIn = sinon.stub().returns(false) this.middleware(this.req, this.res, this.next) }) diff --git a/services/web/test/unit/src/Collaborators/CollaboratorsInviteControllerTests.js b/services/web/test/unit/src/Collaborators/CollaboratorsInviteControllerTests.js index 1ba43146a5..785cd85340 100644 --- a/services/web/test/unit/src/Collaborators/CollaboratorsInviteControllerTests.js +++ b/services/web/test/unit/src/Collaborators/CollaboratorsInviteControllerTests.js @@ -92,6 +92,10 @@ describe('CollaboratorsInviteController', function () { addEntryInBackground: sinon.stub(), } + this.AuthenticationController = { + setRedirectInSession: sinon.stub(), + } + this.CollaboratorsInviteController = SandboxedModule.require(MODULE_PATH, { requires: { '../Project/ProjectGetter': this.ProjectGetter, @@ -105,6 +109,8 @@ describe('CollaboratorsInviteController', function () { '../Authentication/SessionManager': this.SessionManager, '@overleaf/settings': this.settings, '../../infrastructure/RateLimiter': this.RateLimiter, + '../Authentication/AuthenticationController': + this.AuthenticationController, }, }) @@ -626,6 +632,35 @@ describe('CollaboratorsInviteController', function () { }) }) + describe('when not logged in', function () { + beforeEach(function (done) { + this.SessionManager.getSessionUser.returns(null) + + this.res.callback = () => done() + this.CollaboratorsInviteController.viewInvite( + this.req, + this.res, + this.next + ) + }) + it('should not check member status', function () { + expect(this.CollaboratorsGetter.promises.isUserInvitedMemberOfProject) + .to.not.have.been.called + }) + + it('should set redirect back to invite', function () { + expect( + this.AuthenticationController.setRedirectInSession + ).to.have.been.calledWith(this.req) + }) + + it('should redirect to the register page', function () { + expect(this.res.render).to.not.have.been.called + expect(this.res.redirect).to.have.been.calledOnce + expect(this.res.redirect).to.have.been.calledWith('/register') + }) + }) + describe('when user is already a member of the project', function () { beforeEach(function (done) { this.CollaboratorsGetter.promises.isUserInvitedMemberOfProject.resolves( diff --git a/services/web/test/unit/src/Email/EmailBuilderTests.js b/services/web/test/unit/src/Email/EmailBuilderTests.js index e4dde40dfa..1f7628bc3f 100644 --- a/services/web/test/unit/src/Email/EmailBuilderTests.js +++ b/services/web/test/unit/src/Email/EmailBuilderTests.js @@ -537,12 +537,7 @@ describe('EmailBuilder', function () { } this.projectName = 'Top Secret' this.opts = { - inviteUrl: - `${this.settings.siteUrl}/project/projectId123/invite/token/aToken123?` + - [ - `project_name=${encodeURIComponent(this.projectName)}`, - `user_first_name=${encodeURIComponent(this.owner.name)}`, - ].join('&'), + inviteUrl: `${this.settings.siteUrl}/project/projectId123/invite/token/aToken123`, owner: { email: this.owner.email, }, diff --git a/services/web/test/unit/src/User/UserPagesControllerTests.js b/services/web/test/unit/src/User/UserPagesControllerTests.js index 7e94839543..299a3e8f54 100644 --- a/services/web/test/unit/src/User/UserPagesControllerTests.js +++ b/services/web/test/unit/src/User/UserPagesControllerTests.js @@ -119,8 +119,10 @@ describe('UserPagesController', function () { }) it('should set sharedProjectData', function (done) { - this.req.query.project_name = 'myProject' - this.req.query.user_first_name = 'user_first_name_here' + this.req.session.sharedProjectData = { + project_name: 'myProject', + user_first_name: 'user_first_name_here', + } this.res.callback = () => { this.res.renderedVariables.sharedProjectData.project_name.should.equal(