Merge pull request #18294 from overleaf/jpa-td-invite-details

[web] avoid content reflection via query parameter on register page

GitOrigin-RevId: 43e7ba6069e0d9f3f12e5e9e680b5960b0673782

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)

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) {

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,12 +235,14 @@ 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)
+    if (currentUser) {
       const isMember =
         await CollaboratorsGetter.promises.isUserInvitedMemberOfProject(
           currentUser._id,
@@ -252,6 +255,7 @@ const CollaboratorsInviteController = {
         )
         return res.redirect(`/project/${projectId}`)
       }
+    }
 
     // get the invite
     const invite = await CollaboratorsInviteHandler.promises.getInviteByToken(
@@ -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,

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()
     )

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<void>}
  */
 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

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) {

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(/<title>Invalid Invite - .*<\/title>/)
     callback()
   })
@@ -232,8 +215,16 @@ const expectInviteRedirectToRegister = (user, link, callback) => {
     expect(err).not.to.exist
     expect(response.statusCode).to.equal(302)
     expect(response.headers.location).to.match(/^\/register.*$/)
+
+    user.getSession((err, session) => {
+      if (err) return callback(err)
+      expect(session.sharedProjectData).deep.equals({
+        project_name: PROJECT_NAME,
+        user_first_name: OWNER_NAME,
+      })
       callback()
     })
+  })
 }
 
 const expectLoginPage = (user, callback) => {
@@ -253,12 +244,27 @@ 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)
+
+    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()
+    }
+  })
 }
 
 const expectInviteRedirectToProject = (user, link, invite, 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),
             ],

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)
                 })
               }
             )

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)
       })

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(

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,
             },

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(