From f76563787b1a29d6464ede02bbb01bebe1028347 Mon Sep 17 00:00:00 2001 From: Jessica Lawshe <5312836+lawshe@users.noreply.github.com> Date: Mon, 20 Nov 2023 07:54:22 -0600 Subject: [PATCH] Merge pull request #15838 from overleaf/jel-link-sharing-redirect-hash [web] Save link sharing URL hash as part of redirect GitOrigin-RevId: 7d067852863b93e3246e5132511031005e333810 --- .../TokenAccess/TokenAccessController.js | 15 +++-- .../TokenAccess/TokenAccessHandler.js | 5 +- .../test/acceptance/src/TokenAccessTests.js | 67 +++++++++++++++++++ .../TokenAccess/TokenAccessControllerTests.js | 44 ++++++------ .../TokenAccess/TokenAccessHandlerTests.js | 27 +------- 5 files changed, 105 insertions(+), 53 deletions(-) diff --git a/services/web/app/src/Features/TokenAccess/TokenAccessController.js b/services/web/app/src/Features/TokenAccess/TokenAccessController.js index 6802f519c7..c311929793 100644 --- a/services/web/app/src/Features/TokenAccess/TokenAccessController.js +++ b/services/web/app/src/Features/TokenAccess/TokenAccessController.js @@ -111,6 +111,7 @@ async function checkAndGetProjectOrResponseAction( tokenType, token, userId, + tokenHashPrefix, req, res, next @@ -122,10 +123,14 @@ async function checkAndGetProjectOrResponseAction( !TokenAccessHandler.ANONYMOUS_READ_AND_WRITE_ENABLED ) { logger.warn('[TokenAccess] deny anonymous read-and-write token access') - AuthenticationController.setRedirectInSession( - req, - TokenAccessHandler.makeTokenUrl(token) - ) + + let projectUrlWithToken = TokenAccessHandler.makeTokenUrl(token) + + if (tokenHashPrefix && tokenHashPrefix.startsWith('#')) { + projectUrlWithToken += `${tokenHashPrefix}` + } + + AuthenticationController.setRedirectInSession(req, projectUrlWithToken) return [ null, () => { @@ -245,6 +250,7 @@ async function grantTokenAccessReadAndWrite(req, res, next) { tokenType, token, userId, + tokenHashPrefix, req, res, next @@ -323,6 +329,7 @@ async function grantTokenAccessReadOnly(req, res, next) { tokenType, token, userId, + tokenHashPrefix, req, res, next diff --git a/services/web/app/src/Features/TokenAccess/TokenAccessHandler.js b/services/web/app/src/Features/TokenAccess/TokenAccessHandler.js index 6da0d7f0cf..081ea24ef3 100644 --- a/services/web/app/src/Features/TokenAccess/TokenAccessHandler.js +++ b/services/web/app/src/Features/TokenAccess/TokenAccessHandler.js @@ -310,10 +310,7 @@ const TokenAccessHandler = { } } - if ( - hashPrefixStatus === 'mismatch' || - hashPrefixStatus === 'mismatch-v1-format' - ) { + if (hashPrefixStatus === 'mismatch') { logger.info( { tokenHashPrefix, diff --git a/services/web/test/acceptance/src/TokenAccessTests.js b/services/web/test/acceptance/src/TokenAccessTests.js index d8e548113a..453e78e06b 100644 --- a/services/web/test/acceptance/src/TokenAccessTests.js +++ b/services/web/test/acceptance/src/TokenAccessTests.js @@ -1247,6 +1247,73 @@ describe('TokenAccess', function () { done ) }) + + it('should save URL hash in redirect', function (done) { + const urlFragment = '#123456' + const tokenWithUrlFragment = `${this.tokens.readAndWrite}${urlFragment}` + + async.series( + [ + cb => + tryEditorAccess( + this.anon, + this.projectId, + expectErrorResponse.restricted.html, + cb + ), + cb => + this.anon.request.get( + tokenWithUrlFragment, + (err, response, body) => { + if (err) { + return cb(err) + } + expect(response.statusCode).to.equal(200) + + this.anon.request.post( + `${this.tokens.readAndWrite}/grant`, + { + json: { + token: this.tokens.readAndWrite, + tokenHashPrefix: urlFragment, + }, + }, + (err, response, body) => { + if (err) { + return cb(err) + } + expect(response.statusCode).to.equal(200) + expect(body).to.deep.equal({ + redirect: '/restricted', + anonWriteAccessDenied: true, + }) + cb() + } + ) + } + ), + cb => + tryAnonContentAccess( + this.anon, + this.projectId, + this.tokens.readAndWrite, + (response, body) => { + expect(response.statusCode).to.equal(403) + expect(body).to.equal('Forbidden') + }, + cb + ), + cb => + this.anon.login((err, response, body) => { + expect(err).to.not.exist + expect(response.statusCode).to.equal(200) + expect(body.redir).to.equal(`/${tokenWithUrlFragment}`) + cb() + }), + ], + done + ) + }) }) } else { describe('anonymous read-and-write token, enabled', function () { diff --git a/services/web/test/unit/src/TokenAccess/TokenAccessControllerTests.js b/services/web/test/unit/src/TokenAccess/TokenAccessControllerTests.js index 4e57ba0e38..830a17b73f 100644 --- a/services/web/test/unit/src/TokenAccess/TokenAccessControllerTests.js +++ b/services/web/test/unit/src/TokenAccess/TokenAccessControllerTests.js @@ -86,7 +86,7 @@ describe('TokenAccessController', function () { describe('normal case', function () { beforeEach(function (done) { this.req.params = { token: this.token } - this.req.body = { confirmedByUser: true, tokenHashPrefix: 'prefix' } + this.req.body = { confirmedByUser: true, tokenHashPrefix: '#prefix' } this.res.callback = done this.TokenAccessController.grantTokenAccessReadAndWrite( this.req, @@ -118,7 +118,7 @@ describe('TokenAccessController', function () { this.TokenAccessHandler.checkTokenHashPrefix ).to.have.been.calledWith( this.token, - 'prefix', + '#prefix', 'readAndWrite', this.user._id, { projectId: this.project._id, action: 'continue' } @@ -222,7 +222,7 @@ describe('TokenAccessController', function () { beforeEach(function () { this.SessionManager.getLoggedInUserId.returns(null) this.req.params = { token: this.token } - this.req.body = { tokenHashPrefix: 'prefix' } + this.req.body = { tokenHashPrefix: '#prefix' } }) describe('ANONYMOUS_READ_AND_WRITE_ENABLED is undefined', function () { beforeEach(function (done) { @@ -246,7 +246,7 @@ describe('TokenAccessController', function () { this.TokenAccessHandler.checkTokenHashPrefix ).to.have.been.calledWith( this.token, - 'prefix', + '#prefix', 'readAndWrite', null, { @@ -254,6 +254,12 @@ describe('TokenAccessController', function () { } ) }) + + it('saves redirect URL with URL fragment', function () { + expect( + this.AuthenticationController.setRedirectInSession.lastCall.args[1] + ).to.equal('/#prefix') + }) }) describe('ANONYMOUS_READ_AND_WRITE_ENABLED is true', function () { @@ -280,7 +286,7 @@ describe('TokenAccessController', function () { this.TokenAccessHandler.checkTokenHashPrefix ).to.have.been.calledWith( this.token, - 'prefix', + '#prefix', 'readAndWrite', null, { @@ -304,7 +310,7 @@ describe('TokenAccessController', function () { has_owner: true, }) this.req.params = { token: this.token } - this.req.body = { tokenHashPrefix: 'prefix' } + this.req.body = { tokenHashPrefix: '#prefix' } this.res.callback = done this.TokenAccessController.grantTokenAccessReadAndWrite( this.req, @@ -328,7 +334,7 @@ describe('TokenAccessController', function () { this.TokenAccessHandler.checkTokenHashPrefix ).to.have.been.calledWith( this.token, - 'prefix', + '#prefix', 'readAndWrite', this.user._id, { @@ -345,7 +351,7 @@ describe('TokenAccessController', function () { exists: false, }) this.req.params = { token: this.token } - this.req.body = { tokenHashPrefix: 'prefix' } + this.req.body = { tokenHashPrefix: '#prefix' } this.res.callback = done this.TokenAccessController.grantTokenAccessReadAndWrite( this.req, @@ -361,7 +367,7 @@ describe('TokenAccessController', function () { this.TokenAccessHandler.checkTokenHashPrefix ).to.have.been.calledWith( this.token, - 'prefix', + '#prefix', 'readAndWrite', this.user._id, { @@ -376,7 +382,7 @@ describe('TokenAccessController', function () { beforeEach(function () { this.TokenAccessHandler.promises.getProjectByToken.resolves(undefined) this.req.params = { token: this.token } - this.req.body = { tokenHashPrefix: 'prefix' } + this.req.body = { tokenHashPrefix: '#prefix' } }) it('passes Errors.NotFoundError to next when project not found and still checks token hash', function (done) { this.TokenAccessController.grantTokenAccessReadAndWrite( @@ -389,7 +395,7 @@ describe('TokenAccessController', function () { this.TokenAccessHandler.checkTokenHashPrefix ).to.have.been.calledWith( this.token, - 'prefix', + '#prefix', 'readAndWrite', this.user._id, { @@ -406,7 +412,7 @@ describe('TokenAccessController', function () { it('passes Errors.NotFoundError to next when token access is not enabled but still checks token hash', function (done) { this.TokenAccessHandler.tokenAccessEnabledForProject.returns(false) this.req.params = { token: this.token } - this.req.body = { tokenHashPrefix: 'prefix' } + this.req.body = { tokenHashPrefix: '#prefix' } this.TokenAccessController.grantTokenAccessReadAndWrite( this.req, this.res, @@ -417,7 +423,7 @@ describe('TokenAccessController', function () { this.TokenAccessHandler.checkTokenHashPrefix ).to.have.been.calledWith( this.token, - 'prefix', + '#prefix', 'readAndWrite', this.user._id, { @@ -434,7 +440,7 @@ describe('TokenAccessController', function () { it('returns 400 when not using a read write token', function () { this.TokenAccessHandler.isReadAndWriteToken.returns(false) this.req.params = { token: this.token } - this.req.body = { tokenHashPrefix: 'prefix' } + this.req.body = { tokenHashPrefix: '#prefix' } this.TokenAccessController.grantTokenAccessReadAndWrite( this.req, this.res @@ -447,7 +453,7 @@ describe('TokenAccessController', function () { describe('normal case', function () { beforeEach(function (done) { this.req.params = { token: this.token } - this.req.body = { confirmedByUser: true, tokenHashPrefix: 'prefix' } + this.req.body = { confirmedByUser: true, tokenHashPrefix: '#prefix' } this.res.callback = done this.TokenAccessController.grantTokenAccessReadOnly( this.req, @@ -479,7 +485,7 @@ describe('TokenAccessController', function () { this.TokenAccessHandler.checkTokenHashPrefix ).to.have.been.calledWith( this.token, - 'prefix', + '#prefix', 'readOnly', this.user._id, { projectId: this.project._id, action: 'continue' } @@ -521,7 +527,7 @@ describe('TokenAccessController', function () { it('returns 400 when not using a read only token', function () { this.TokenAccessHandler.isReadOnlyToken.returns(false) this.req.params = { token: this.token } - this.req.body = { tokenHashPrefix: 'prefix' } + this.req.body = { tokenHashPrefix: '#prefix' } this.TokenAccessController.grantTokenAccessReadOnly(this.req, this.res) expect(this.res.sendStatus).to.have.been.calledWith(400) }) @@ -587,7 +593,7 @@ describe('TokenAccessController', function () { it('passes Errors.NotFoundError to next when token access is not enabled but still checks token hash', function (done) { this.TokenAccessHandler.tokenAccessEnabledForProject.returns(false) this.req.params = { token: this.token } - this.req.body = { tokenHashPrefix: 'prefix' } + this.req.body = { tokenHashPrefix: '#prefix' } this.TokenAccessController.grantTokenAccessReadOnly( this.req, this.res, @@ -598,7 +604,7 @@ describe('TokenAccessController', function () { this.TokenAccessHandler.checkTokenHashPrefix ).to.have.been.calledWith( this.token, - 'prefix', + '#prefix', 'readOnly', this.user._id, { diff --git a/services/web/test/unit/src/TokenAccess/TokenAccessHandlerTests.js b/services/web/test/unit/src/TokenAccess/TokenAccessHandlerTests.js index 02c4757312..94ce66ee3f 100644 --- a/services/web/test/unit/src/TokenAccess/TokenAccessHandlerTests.js +++ b/services/web/test/unit/src/TokenAccess/TokenAccessHandlerTests.js @@ -755,10 +755,10 @@ describe('TokenAccessHandler', function () { } ) }) + it('sends "mismatch-v1-format" for suspected v1 URLs with 7 numbers in URL fragment', function () { const token = '4112142489ddsbkrdzhxrq' const prefix = '%2F1234567%2F' - this.TokenAccessHandler.checkTokenHashPrefix( token, `#${prefix}`, @@ -766,7 +766,6 @@ describe('TokenAccessHandler', function () { userId, { projectId } ) - expect(this.Metrics.inc).to.have.been.calledWith( 'link-sharing.hash-check', { @@ -774,22 +773,10 @@ describe('TokenAccessHandler', function () { status: 'mismatch-v1-format', } ) - - expect(this.logger.info).to.have.been.calledWith( - { - tokenHashPrefix: prefix, - hashPrefixStatus: 'mismatch-v1-format', - userId, - projectId, - type: 'readAndWrite', - }, - 'mismatched token hash prefix' - ) }) it('sends "mismatch-v1-format" for suspected v1 URLs with 8 numbers in URL fragment', function () { const token = '4112142489ddsbkrdzhxrq' const prefix = '%2F12345678%2F' - this.TokenAccessHandler.checkTokenHashPrefix( token, `#${prefix}`, @@ -797,7 +784,6 @@ describe('TokenAccessHandler', function () { userId, { projectId } ) - expect(this.Metrics.inc).to.have.been.calledWith( 'link-sharing.hash-check', { @@ -805,17 +791,6 @@ describe('TokenAccessHandler', function () { status: 'mismatch-v1-format', } ) - - expect(this.logger.info).to.have.been.calledWith( - { - tokenHashPrefix: prefix, - hashPrefixStatus: 'mismatch-v1-format', - userId, - projectId, - type: 'readAndWrite', - }, - 'mismatched token hash prefix' - ) }) }) })